[webkit-reviews] review denied: [Bug 63565] DFG non-speculative JIT does not reuse registers when compiling comparisons : [Attachment 98975] the patch

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 63565: DFG non-speculative JIT does not reuse registers when compiling
comparisons
https://bugs.webkit.org/show_bug.cgi?id=63565

Attachment 98975: the patch
https://bugs.webkit.org/attachment.cgi?id=98975&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
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.