[webkit-reviews] review denied: [Bug 183130] [MIPS] Optimize generated JIT code for branches : [Attachment 335174] MIPS branches

[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 183130: [MIPS] Optimize generated JIT code for branches
https://bugs.webkit.org/show_bug.cgi?id=183130

Attachment 335174: MIPS branches

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




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

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

> Source/JavaScriptCore/ChangeLog:15
> +	   on a MIPS platform. Firstly, the patch
https://bugs.webkit.org/show_bug.cgi?id=101328
> +	   added two nop instructions to branchEqual() and branchNotEqual() in
order to prevent
> +	   the beq/bne instruction in a patchable branch from being overwritten
by replaceWithJump()
> +	   in case when we have to jump outside the 256MB-aligned block.
However, this adds a significant
> +	   overhead for all other types of branches. Since these nop's protect
the code that comes after
> +	   the code generated by moveWithPatch(), this function seems like a
better place to add them.
> +	   Secondly, in branches with immediate operand, the immediate value is
0 in many cases, and in

What happens if we just use patchableBranch64 and repatch it?
I don't think this is safe if it becomes broken.
How about separating patchableBranchPtr, patchableBranchPtrWithPatch etc. from
branchPtr implementation and emitting nops in patchable versions?

> Source/JavaScriptCore/assembler/MIPSAssembler.h:1016
> +	   cacheFlush(insn - 2, codeSize);

Lol, nice. I think `cacheFlush(instructionStart, codeSize);` is nicer.
BTW, replaceWithLoad and replaceWithAddressComputation should be fixed at
least. Could you review all the other replacing functions too?

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1834
>	       */

Let's change the comment.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1887
>	       */

Ditto.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1961
>	       if (imm.m_value >= -32768 && imm.m_value  <= 32767 &&
!m_fixedWidth) {

Ditto.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2025
>	       m_assembler.mfhi(dataTempRegister);

Ditto.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2070
>	       m_assembler.mfhi(dataTempRegister);

Ditto.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2120
>	       */

Ditto.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2179
>	       */

Ditto.