[webkit-reviews] review denied: [Bug 183243] [MIPS] Optimize generated JIT code for loads/stores : [Attachment 335846] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has denied Stanislav Ocovaj
<stanislav.ocovaj at rt-rk.com>'s request for review:
Bug 183243: [MIPS] Optimize generated JIT code for loads/stores
https://bugs.webkit.org/show_bug.cgi?id=183243

Attachment 335846: Patch

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




--- Comment #8 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 335846
  --> https://bugs.webkit.org/attachment.cgi?id=335846
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335846&action=review

Overall, looks good to me. But r- because I found one bug in this patch.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:269
> +	       if ((adr >> 15) == ((adr+4) >> 15)) {

Style nits: use `(adr + 4)` instead of `(adr+4)`.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:280
> +		   m_assembler.lw(dataTempRegister, addrTempRegister, (adr+4) &
0xffff);

Ditto.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:284
> +		   m_assembler.sw(dataTempRegister, addrTempRegister, (adr+4) &
0xffff);

Ditto.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:709
> +		  li   addrTemp, address
> +		  li   immTemp, imm
> +		  lw   dataTemp, 0(addrTemp)
> +		  subu dataTemp, dataTemp, immTemp
> +		  sw   dataTemp, 0(addrTemp)

We need to fix this to align to the actual code.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:807
> +	   }

It is wrong. ConvertibleLoadLabel's instructions need to be convertible to
addPtr instruction sequence
Please check MIPSAssembler::replaceWithLoad too.