Bug 183130 – [MIPS] Optimize generated JIT code for branches

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Stanislav Ocovaj]

There are a couple of places where a more efficient code can be generated for branches 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 these cases we can use the zero register directly without generating a move (pseudo)instruction.

[Stanislav Ocovaj]

Created attachment 334617 [details]
MIPS branches

This patch implements the proposed optimizations

[Stanislav Ocovaj]

The uploaded patch also fixes a bug in revertJumpToMove() where an incorrect pointer is passed to cacheFlush().
The implemented optimizations improve the benchmark results by ~5%

[Petar Jovanovic]

Can anyone take a look at this change?

[Guillaume Emont]

The patch should be flagged as "r?" to attract the attention of reviewers.

[Mark Lam]

(In reply to Guillaume Emont from comment #4)
> The patch should be flagged as "r?" to attract the attention of reviewers.

Also, this patch is also all in MIPS code.  I think it is better for someone who knows MIPS to do the review.  Are there any reviewers with MIPS expertise?

[Stanislav Ocovaj]

Created attachment 335174 [details]
MIPS branches

Rebased and added details to ChangeLog

[Petar Jovanovic]

Anyone?

[Yusuke Suzuki]

Comment on attachment 335174 [details]
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.

[Yusuke Suzuki]

Comment on attachment 335174 [details]
MIPS branches

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

>> Source/JavaScriptCore/ChangeLog:15
>> +        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?

I mean, using replacing function with PatchableJump which does not come after moveWithPatch.

[Stanislav Ocovaj]

Comment on attachment 335174 [details]
MIPS branches

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

>> 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?

replaceWithLoad and replaceWithAddressComputation are OK because they only modify the last instruction. Other functions are also correct. It think repatchInt32 could be modified too, to use cacheFlush(from... instead of cacheFlush(insn... and remove the insn-- statement.

[Stanislav Ocovaj]

Sorry, I think my explanation was not clear enough. The patch https://bugs.webkit.org/show_bug.cgi?id=101328 added two nop's to branchEqual() and branchNotEqual() in order to fix the following issue.
Without the nop's, branchPtrWithPatch() generates a block of instructions like this:

  lui immTempRegister, initialRightValue >> 16
  ori immTempRegister, immTempRegister, initialRightValue & 0xffff
  beq left, immTempRegister, label
  nop
  beq zero, zero, 1
  nop
  nop
  nop
1:

Then, at some point, this code may be changed by replaceWithJump() to

  lui t9, newLabel >> 16
  ori t9, newLabel & 0xffff
  jr  t9
  nop
  beq zero, zero, 1
  nop
  nop
  nop
1:

which overwrites the original beq instruction. Later on, the JIT may want to revert the jump back to branchPtrWithPatch by calling revertJumpReplacementToBranchPtrWithPatch(), which results with the following block:

  lui immTempRegister, initialRightValue >> 16
  ori immTempRegister, immTempRegister, initialRightValue & 0xffff
  nop
  nop
  beq zero, zero, 1
  nop
  nop
  nop
1:

Since the original beq instruction is lost, this code won't work anymore. So, the added nop's have nothing to do with patchable branches actually, they were added in orded to allow the code generated by branchPtrWithPatch() to be reverted back to branchPtrWithPatch after replacing it with a 4-instruction jump.
Repatching branches only modifies the last 6 instructions of the branch (see relinkJump() in MIPSAssembler.h), so that should not be affected.

[Stanislav Ocovaj]

Created attachment 336192 [details]
Patch

I added the requested changes and rebased the patch. I removed the optimizations for branches with immediate operand as they are now included in another patch (https://bugs.webkit.org/show_bug.cgi?id=183740). I also moved the additional nop's from moveWithPatch to branchPtrWithPatch since that is the only place where they are really needed.

[Stanislav Ocovaj]

Just for the record, with this optimization I got the following results on a mips32r1 platform:
Benchmark  Original  Optimized  Improvement
Octane2    845       897        6.2%
Kraken     3306 ms   3133 ms    5.5%
JetStream  7.13      7.56       6.0%

[Petar Jovanovic]

Ping.

[Stanislav Ocovaj]

Created attachment 337166 [details]
Patch

Rebased

[Petar Jovanovic]

Yusuke Suzuki, are you OK with the change?

[Yusuke Suzuki]

Comment on attachment 337166 [details]
Patch

OK, r=me.

[WebKit Commit Bot]

Comment on attachment 337166 [details]
Patch

Clearing flags on attachment: 337166

Committed r230310: <https://trac.webkit.org/changeset/230310>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39215379>