Bug 63565 – DFG non-speculative JIT does not reuse registers when compiling comparisons

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

[Filip Pizlo]

The DFG JIT generally attempts to reuse argument registers for temporaries and results, particularly when there are obvious opportunities to do so.  The DFG non-speculative JIT fails to reuse registers when emitting code for comparisons.  Moreover, when it does reuse registers, it emits spurious and unnecessary zero-extend sequences even though the subsequent operation will perform zero-extension automatically.

[Filip Pizlo]

Created attachment 98975 [details]
the patch

[Gavin Barraclough]

Comment on attachment 98975 [details]
the patch

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

r- based on above comments, I think we can simplify further by switching to the 3-op branchAdd32 etc calls.

In basicArithOp we're also retagging arg2 in the case of overflow (& a register has been reused), which I think we can remove if we stop zero-extending the operands.

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:-176
> -        

Actually, I think we can just remove this - the macro assembler has a 3-op form:
branchAdd32(ResultCondition cond, RegisterID src, TrustedImm32 imm, RegisterID dest) (& branchSub32).
These should be more helpful for platforms with 3-op instructions, like arm.

BTW, if we didn't have the 3-op branchAdd32, then I don't think you need zero extension here (since 32bit ops ignore the high bits coming in, and always set high bits to zero), so I think you could just use a move.  And the macro assembler will automatically elide moves to the same register, so you wouldn't need the if check.

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:291
>      m_jit.zeroExtend32ToPtr(arg1GPR, resultGPR);

Looking over this, again, I don't think we need the zero extends here, and as such, I don't think we need use to use a temporary, at minimum in the case of no overflow.  Again, there are 3-op branchAdd/Sub/Mul calls, which should allow us to plant this in a single instruction on arm.

[Filip Pizlo]

Created attachment 99019 [details]
the patch (fix review)

[WebKit Review Bot]

Comment on attachment 99019 [details]
the patch (fix review)

Clearing flags on attachment: 99019

Committed r90189: <http://trac.webkit.org/changeset/90189>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.