Add new or32 implementation for MIPS after r194613
Created attachment 268495 [details] Patch
Comment on attachment 268495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268495&action=review > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:384 > + if (imm.m_value > 0 && imm.m_value < 65535 I'm no mips expert, but should this be <= 65535? There are other places in MacroAssemblerMIPS.h where this is also < 65535, but don't they all work using the 16-bit immediate value if the immediate value is equal to 65535, too?
Comment on attachment 268495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268495&action=review >> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:384 >> + if (imm.m_value > 0 && imm.m_value < 65535 Please use a or32(TrustedImm32, RegisterID) call here to avoid code duplication, something like this: load32(dest.m_ptr, immTempRegister); or32(imm, dest); store32(immTempRegister, dest.m_ptr); > I'm no mips expert, but should this be <= 65535? There are other places in MacroAssemblerMIPS.h where this is also < 65535, but don't they all work using the 16-bit immediate value if the immediate value is equal to 65535, too? You're right Alex, I also think this should be <= 0xffff. As there are other places in MacroAssemblerMIPS needing this change, maybe we could do this in a separate change ?
Created attachment 268532 [details] Patch
Comment on attachment 268532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268532&action=review LGTM > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:381 > + // TODO: Swap dataTempRegister and immTempRegister usage Can you just add the (imm value == 0) check here to avoid load32/store32 in this case: if (!imm.m_value && !m_fixedWidth) return;
Created attachment 268533 [details] Patch
LGTM, thanks
Comment on attachment 268533 [details] Patch r=me
Comment on attachment 268533 [details] Patch Clearing flags on attachment: 268533 Committed r194763: <http://trac.webkit.org/changeset/194763>
All reviewed patches have been landed. Closing bug.