[webkit-reviews] review granted: [Bug 183740] [MIPS] Optimize JIT code generated by methods with TrustedImm32 operand : [Attachment 336038] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted Stanislav Ocovaj
<stanislav.ocovaj at rt-rk.com>'s request for review:
Bug 183740: [MIPS] Optimize JIT code generated by methods with TrustedImm32
operand
https://bugs.webkit.org/show_bug.cgi?id=183740

Attachment 336038: Patch

https://bugs.webkit.org/attachment.cgi?id=336038&action=review




--- Comment #4 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 336038
  --> https://bugs.webkit.org/attachment.cgi?id=336038
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 ;)