Fix bug with GVN on array loads.

Fix bug with GVN on array loads.

This fixes a bug where an array load was incorrectly hoisted by GVN.

BUG=85177
TEST=mjsunit/regress/regress-85177.js
Committed: http://code.google.com/p/v8/source/detail?r=8230

[Mads Ager (chromium), 2011-06-08 12:27:25]

LGTM!

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (left):

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:1737: ASSERT(HasNoUses());
Maybe we can even assert that all bounds check must have uses?

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen.cc#newcode3728
src/hydrogen.cc:3728: AddInstruction(checked_key = new(zone()) HBoundsCheck(key,
length));
To match the line above can we do:

checked_key = AddInstruction(new(zone()) H...); ?

I don't like the assignment in the arguments list. So I would pull that out in
any case.

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen.cc#newcode3733
src/hydrogen.cc:3733: AddInstruction(checked_key = new(zone()) HBoundsCheck(key,
length));
Ditto.

[fschneider, 2011-06-08 12:33:54]

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (left):

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:1737: ASSERT(HasNoUses());
On 2011/06/08 12:27:25, Mads Ager wrote:
> Maybe we can even assert that all bounds check must have uses?

Done.

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen.cc#newcode3728
src/hydrogen.cc:3728: AddInstruction(checked_key = new(zone()) HBoundsCheck(key,
length));
On 2011/06/08 12:27:25, Mads Ager wrote:
> To match the line above can we do:
> 
> checked_key = AddInstruction(new(zone()) H...); ?
> 
> I don't like the assignment in the arguments list. So I would pull that out in
> any case.

Done.

http://codereview.chromium.org/7003054/diff/5002/src/hydrogen.cc#newcode3733
src/hydrogen.cc:3733: AddInstruction(checked_key = new(zone()) HBoundsCheck(key,
length));
On 2011/06/08 12:27:25, Mads Ager wrote:
> Ditto.

Done.

[fschneider, 2011-06-08 13:03:17]

Could you have another quick look at hydrogen.cc?

I changed all the other places where we do bounds checks.

[Mads Ager (chromium), 2011-06-08 13:20:20]

Still LGTM. Thanks.

[fschneider, 2011-06-09 09:46:40]

This patch set fixes the performance regression introduced by my first fix.

[Mads Ager (chromium), 2011-06-09 10:58:50]

LGTM

http://codereview.chromium.org/7003054/diff/5004/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (left):

http://codereview.chromium.org/7003054/diff/5004/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:1737: ASSERT(HasNoUses());
It seemed like a very good idea to have the assert here. Why is it gone in this
change?

[fschneider, 2011-06-09 11:03:52]

http://codereview.chromium.org/7003054/diff/5004/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (left):

http://codereview.chromium.org/7003054/diff/5004/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:1737: ASSERT(HasNoUses());
On 2011/06/09 10:58:50, Mads Ager wrote:
> It seemed like a very good idea to have the assert here. Why is it gone in
this
> change?

The problem is that HBoundsCheck does only have uses until we remove them in the
last phase of the graph builder, so I would need to say sth like

ASSERT(UseCount() > 0 || gvn_done)

[Mads Ager (chromium), 2011-06-09 11:12:00]

On 2011/06/09 11:03:52, fschneider wrote:
> http://codereview.chromium.org/7003054/diff/5004/src/hydrogen-instructions.cc
> File src/hydrogen-instructions.cc (left):
> 
>
http://codereview.chromium.org/7003054/diff/5004/src/hydrogen-instructions.cc...
> src/hydrogen-instructions.cc:1737: ASSERT(HasNoUses());
> On 2011/06/09 10:58:50, Mads Ager wrote:
> > It seemed like a very good idea to have the assert here. Why is it gone in
> this
> > change?
> 
> The problem is that HBoundsCheck does only have uses until we remove them in
the
> last phase of the graph builder, so I would need to say sth like
> 
> ASSERT(UseCount() > 0 || gvn_done)

Can we assert when replacing checks that they have uses at that point?