Bug 19484 - More instructions needs to use temporary registers
Summary: More instructions needs to use temporary registers
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-11 09:32 PDT by Cameron Zwarich (cpst)
Modified: 2012-09-11 18:45 PDT (History)
4 users (show)

See Also:


Attachments
Patch towards a fix (8.87 KB, patch)
2008-06-13 21:32 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Fix for AddNode and SubNode (8.74 KB, patch)
2008-06-14 00:16 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Patch to improve codegen (11.99 KB, patch)
2008-06-14 22:16 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Patch for binary operations (67.01 KB, patch)
2008-06-15 01:06 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-06-11 09:32:13 PDT
The code

var a = 1;
print(a + ++a);

prints 4 instead of 3, because we are not moving 'a' to a temporary registers. I fixed this for assignment nodes, but a similar thing needs to be done for a lot more nodes. I'll try to fix this for binary operations today and hope that it doesn't cause too much of a performance regression.
Comment 1 Cameron Zwarich (cpst) 2008-06-13 21:32:36 PDT
Created attachment 21692 [details]
Patch towards a fix

Here is the first step towards a solution. It makes codegen of expressions that might need a temporary for the LHS smarter when the RHS is a constant. It's not that big of a deal now, but it will matter more when we do this for more opcodes.
Comment 2 Maciej Stachowiak 2008-06-13 21:34:45 PDT
Comment on attachment 21692 [details]
Patch towards a fix

r=me
Comment 3 Cameron Zwarich (cpst) 2008-06-13 21:42:10 PDT
Landed in r34528.
Comment 4 Cameron Zwarich (cpst) 2008-06-14 00:16:39 PDT
Created attachment 21695 [details]
Fix for AddNode and SubNode

Here is a fix for the AddNode and SubNode case. It only adds one instruction in SunSpider, for the line

dnaInput = dnaInput + dnaInput + dnaInput;

in global code in regexp-dna.
Comment 5 Oliver Hunt 2008-06-14 00:38:50 PDT
Comment on attachment 21695 [details]
Fix for AddNode and SubNode

r=me
Comment 6 Cameron Zwarich (cpst) 2008-06-14 00:54:28 PDT
Landed in r34535.

I plan to do the multiplication nodes next, and finally the bit operations.
Comment 7 Oliver Hunt 2008-06-14 01:05:37 PDT
Alas, you'll also need to deal unto the relational operators.
Comment 8 Cameron Zwarich (cpst) 2008-06-14 22:16:03 PDT
Created attachment 21705 [details]
Patch to improve codegen

This patch makes codegen not use a temporary for the LHS if the RHS is a local variable. This improves code generation on SunSpider in a few places. It was a small regression on my machine, but Maciej says it is fine on his.
Comment 9 Maciej Stachowiak 2008-06-14 22:18:55 PDT
Comment on attachment 21705 [details]
Patch to improve codegen

r=me
Comment 10 Cameron Zwarich (cpst) 2008-06-15 01:06:13 PDT
Created attachment 21706 [details]
Patch for binary operations

Here is a fix for binary operations. I will add tests soon, similar to the other cases I have already landed, but I wanted to put this up for review first.
Comment 11 Maciej Stachowiak 2008-06-15 01:30:14 PDT
Comment on attachment 21706 [details]
Patch for binary operations

r=me
Comment 12 Cameron Zwarich (cpst) 2008-06-15 02:33:32 PDT
Landed in r34562. There are still cases with FunctionCallResolveNode and ForInNode to fix, but I don't know of any others.
Comment 13 Cameron Zwarich (cpst) 2008-06-24 14:38:50 PDT
Comment on attachment 21692 [details]
Patch towards a fix

Clearing review flag.
Comment 14 Cameron Zwarich (cpst) 2008-06-24 14:39:00 PDT
Comment on attachment 21695 [details]
Fix for AddNode and SubNode

Clearing review flag.
Comment 15 Cameron Zwarich (cpst) 2008-06-24 14:39:12 PDT
Comment on attachment 21705 [details]
Patch to improve codegen

Clearing review flag.
Comment 16 Cameron Zwarich (cpst) 2008-06-24 14:39:24 PDT
Comment on attachment 21706 [details]
Patch for binary operations

Clearing review flag.
Comment 17 Alexey Proskuryakov 2010-06-11 17:43:48 PDT
Is this bug still actionable?
Comment 18 Gavin Barraclough 2012-09-11 18:45:10 PDT
All examples given now work, looks like all bugs being tracked here are fixed.  Works for me, if I've missed anything please reopen.