In many macro assembler methods with TrustedImm32 operand a move imm, immTemp (pseudo)instruction is first generated and a register operand variant of the same method is called to generate the rest of the code. If the immediate value can fit in 16 bits then we can skip the move instruction and generate more efficient code using MIPS instructions with immediate operand.
Created attachment 336037 [details] Patch The patch that implements the proposed optimizations
Created attachment 336038 [details] Patch
The patch also fixes a couple of bugs in oveflow detection in branchAdd32(ResultCondition cond, TrustedImm32 imm, AbsoluteAddress dest). Firstly, the condition if (imm.m_value >= -32768 && imm.m_value <= 32767) is inappropriate for xori instruction, which has an unsigned 16-bit immediate operand, unlike the addiu instruction which has a signed 16-bit immediate operand. Secondly, in the else branch, addiu and xori were used instead of addu and xorInsn, with register number being interpreted as an immediate operand. Thirdly, add instruction was placed in the branch delay slot of the first branch instruction, which means the the result would not be stored to the destination address if the branch was taken.
Comment on attachment 336038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336038&action=review r=me with some comments. > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:686 > - if (imm.m_value == -1) { > + if (imm.m_value == -1 && !m_fixedWidth) { > m_assembler.nor(dest, dest, MIPSRegisters::zero); > return; > } > - > - /* > - li immTemp, imm > - xor dest, dest, immTemp > - */ > - move(imm, immTempRegister); > - m_assembler.xorInsn(dest, dest, immTempRegister); > + if (imm.m_value >= 0 && imm.m_value <= 65535 && !m_fixedWidth) > + m_assembler.xori(dest, dest, imm.m_value); > + else { > + /* > + li immTemp, imm > + xor dest, dest, immTemp > + */ > + move(imm, immTempRegister); > + m_assembler.xorInsn(dest, dest, immTempRegister); > + } Let's reorganize this code as follows, if (!m_fixedWidth) { if (imm.m_value == -1) { ... return; } if (imm.m_value >= 0 && imm.m_value <= 65535) { ...; return; } } ...; > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:704 > + if (imm.m_value == -1 && !m_fixedWidth) { > m_assembler.nor(dest, src, MIPSRegisters::zero); > return; > } > - > - /* > - li immTemp, imm > - xor dest, dest, immTemp > - */ > - move(imm, immTempRegister); > - m_assembler.xorInsn(dest, src, immTempRegister); > + if (imm.m_value >= 0 && imm.m_value <= 65535 && !m_fixedWidth) > + m_assembler.xori(dest, src, imm.m_value); > + else { > + /* > + li immTemp, imm > + xor dest, src, immTemp > + */ > + move(imm, immTempRegister); > + m_assembler.xorInsn(dest, src, immTempRegister); > + } Ditto. > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1628 > + if (!m_fixedWidth && !right.m_value) > + return branch32(cond, left, MIPSRegisters::zero); > + if (right.m_value >= -32768 && right.m_value <= 32767 && !m_fixedWidth) { Reorganize the function as follows, if (!m_fixedWidth) { ...; } ...default path...; > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1672 > + if (cond == Equal) { > + move(right, immTempRegister); > + return branchEqual(left, immTempRegister); > + } > + if (cond == NotEqual) { > + move(right, immTempRegister); > + return branchNotEqual(left, immTempRegister); > + } > + if (cond == Above) { > + move(right, immTempRegister); > + m_assembler.sltu(cmpTempRegister, immTempRegister, left); > + return branchNotEqual(cmpTempRegister, MIPSRegisters::zero); > + } > + if (cond == AboveOrEqual) { > + m_assembler.sltiu(cmpTempRegister, left, right.m_value); > + return branchEqual(cmpTempRegister, MIPSRegisters::zero); > + } > + if (cond == Below) { > + m_assembler.sltiu(cmpTempRegister, left, right.m_value); > + return branchNotEqual(cmpTempRegister, MIPSRegisters::zero); > + } > + if (cond == BelowOrEqual) { > + move(right, immTempRegister); > + m_assembler.sltu(cmpTempRegister, immTempRegister, left); > + return branchEqual(cmpTempRegister, MIPSRegisters::zero); > + } > + if (cond == GreaterThan) { > + move(right, immTempRegister); > + m_assembler.slt(cmpTempRegister, immTempRegister, left); > + return branchNotEqual(cmpTempRegister, MIPSRegisters::zero); > + } > + if (cond == GreaterThanOrEqual) { > + m_assembler.slti(cmpTempRegister, left, right.m_value); > + return branchEqual(cmpTempRegister, MIPSRegisters::zero); > + } > + if (cond == LessThan) { > + m_assembler.slti(cmpTempRegister, left, right.m_value); > + return branchNotEqual(cmpTempRegister, MIPSRegisters::zero); > + } > + if (cond == LessThanOrEqual) { > + move(right, immTempRegister); > + m_assembler.slt(cmpTempRegister, immTempRegister, left); > + return branchEqual(cmpTempRegister, MIPSRegisters::zero); > + } Large part of this function is the same to the default code path move(...); return branch32(...); Let's add only special handling here. (AboveOrEqual, Below, GreaterThanOrEqual, LessThan). > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1674 > + ASSERT(0); > + return Jump(); Remove this. > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1768 > + } Reorganize the function as follows, if (!m_fixedWidth) { ...; } ... default path ...; > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1791 > + mask8 = MacroAssemblerHelpers::mask8OnCondition(*this, cond, mask); I think we should not do this here. We should create a helper function mask8OnTest for MacroAssemblerMIPS and use it. For example, test8 should also use it ;)
Created attachment 336278 [details] Patch Patch rebased, requested changes added
Ping.
Comment on attachment 336278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336278&action=review > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2476 > + ASSERT(0); Use ASSERT_NOT_REACHED(); > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2578 > + } Why don't we check m_fixedWidth? > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2612 > void test8(ResultCondition cond, Address address, TrustedImm32 mask, RegisterID dest) Please use mask8OnTest on test8.
Created attachment 336856 [details] Patch
Comment on attachment 336856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336856&action=review > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2141 > + ASSERT(0); Change this to ASSERT_NOT_REACHED(); too.
Created attachment 336982 [details] Patch
Comment on attachment 336982 [details] Patch r=me
Comment on attachment 336982 [details] Patch Clearing flags on attachment: 336982 Committed r230164: <https://trac.webkit.org/changeset/230164>
All reviewed patches have been landed. Closing bug.
<rdar://problem/39110650>