Bug 19484 – More instructions needs to use temporary registers

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: JavaScriptCore

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Maciej Stachowiak]

Comment on attachment 21692 [details]
Patch towards a fix

r=me

[Cameron Zwarich (cpst)]

Landed in r34528.

[Cameron Zwarich (cpst)]

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.

[Oliver Hunt]

Comment on attachment 21695 [details]
Fix for AddNode and SubNode

r=me

[Cameron Zwarich (cpst)]

Landed in r34535.

I plan to do the multiplication nodes next, and finally the bit operations.

[Oliver Hunt]

Alas, you'll also need to deal unto the relational operators.

[Cameron Zwarich (cpst)]

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.

[Maciej Stachowiak]

Comment on attachment 21705 [details]
Patch to improve codegen

r=me

[Cameron Zwarich (cpst)]

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.

[Maciej Stachowiak]

Comment on attachment 21706 [details]
Patch for binary operations

r=me

[Cameron Zwarich (cpst)]

Landed in r34562. There are still cases with FunctionCallResolveNode and ForInNode to fix, but I don't know of any others.

[Cameron Zwarich (cpst)]

Comment on attachment 21692 [details]
Patch towards a fix

Clearing review flag.

[Cameron Zwarich (cpst)]

Comment on attachment 21695 [details]
Fix for AddNode and SubNode

Clearing review flag.

[Cameron Zwarich (cpst)]

Comment on attachment 21705 [details]
Patch to improve codegen

Clearing review flag.

[Cameron Zwarich (cpst)]

Comment on attachment 21706 [details]
Patch for binary operations

Clearing review flag.

[Alexey Proskuryakov]

Is this bug still actionable?

[Gavin Barraclough]

All examples given now work, looks like all bugs being tracked here are fixed.  Works for me, if I've missed anything please reopen.