Bug 183740 – [MIPS] Optimize JIT code generated by methods with TrustedImm32 operand

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Stanislav Ocovaj]

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.

[Stanislav Ocovaj]

Created attachment 336037 [details]
Patch

The patch that implements the proposed optimizations

[Stanislav Ocovaj]

Created attachment 336038 [details]
Patch

[Stanislav Ocovaj]

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.

[Yusuke Suzuki]

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 ;)

[Stanislav Ocovaj]

Created attachment 336278 [details]
Patch

Patch rebased, requested changes added

[Petar Jovanovic]

Ping.

[Yusuke Suzuki]

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.

[Stanislav Ocovaj]

Created attachment 336856 [details]
Patch

[Yusuke Suzuki]

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.

[Stanislav Ocovaj]

Created attachment 336982 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 336982 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 336982 [details]
Patch

Clearing flags on attachment: 336982

Committed r230164: <https://trac.webkit.org/changeset/230164>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39110650>