Bug 63565 - DFG non-speculative JIT does not reuse registers when compiling comparisons
Summary: DFG non-speculative JIT does not reuse registers when compiling comparisons
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-28 14:42 PDT by Filip Pizlo
Modified: 2011-06-30 18:05 PDT (History)
2 users (show)

See Also:


Attachments
the patch (2.63 KB, patch)
2011-06-28 14:46 PDT, Filip Pizlo
barraclough: review-
Details | Formatted Diff | Diff
the patch (fix review) (4.71 KB, patch)
2011-06-28 18:24 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-06-28 14:42:24 PDT
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.
Comment 1 Filip Pizlo 2011-06-28 14:46:30 PDT
Created attachment 98975 [details]
the patch
Comment 2 Gavin Barraclough 2011-06-28 15:07:19 PDT
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.
Comment 3 Filip Pizlo 2011-06-28 18:24:16 PDT
Created attachment 99019 [details]
the patch (fix review)
Comment 4 WebKit Review Bot 2011-06-30 18:05:06 PDT
Comment on attachment 99019 [details]
the patch (fix review)

Clearing flags on attachment: 99019

Committed r90189: <http://trac.webkit.org/changeset/90189>
Comment 5 WebKit Review Bot 2011-06-30 18:05:11 PDT
All reviewed patches have been landed.  Closing bug.