Relax TransitionElementsKind DependsOn/Changes dependencies.

Relax TransitionElementsKind DependsOn/Changes dependencies.

Ensure that GVN eliminates all transitions that are dominated by an equivalent transition, even if there is a DependsOn-changing instruction in between.

R=fschneider@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=10731

[fschneider, 2012-02-15 15:36:48]

The issue description should describe all independent changes:

1. Enable smi-only arrays.

2. Enable GVN of TransitionElementsKind in the case where...

https://chromiumcodereview.appspot.com/9365057/diff/2001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/9365057/diff/2001/src/hydrogen.cc#newc...
src/hydrogen.cc:1196: !value->IsTransitionElementsKind()) {  // Drop it.
I don't understand why skipping transition instructions here is this necessary
(and correct). 
The dependency flags should be enough to control GVN for the
HTransitionElementsKind.

https://chromiumcodereview.appspot.com/9365057/diff/2001/test/mjsunit/element...
File test/mjsunit/elements-transition-hoisting.js (right):

https://chromiumcodereview.appspot.com/9365057/diff/2001/test/mjsunit/element...
test/mjsunit/elements-transition-hoisting.js:186: print("DUDE");
Printing can be removed.

https://chromiumcodereview.appspot.com/9365057/diff/2001/test/mjsunit/element...
test/mjsunit/elements-transition-hoisting.js:197: print("DUDE2");
Printing can be removed.

[danno, 2012-02-16 13:08:52]

Whoops. Wrong version uploaded. I uploaded the right one with your feedback
addressed.

The smi-only flag shouldn't be set at all yet, that was a mistake.

http://codereview.chromium.org/9365057/diff/2001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/9365057/diff/2001/src/hydrogen.cc#newcode1196
src/hydrogen.cc:1196: !value->IsTransitionElementsKind()) {  // Drop it.
On 2012/02/15 15:36:48, fschneider wrote:
> I don't understand why skipping transition instructions here is this necessary
> (and correct). 
> The dependency flags should be enough to control GVN for the
> HTransitionElementsKind.

Done.

http://codereview.chromium.org/9365057/diff/2001/test/mjsunit/elements-transi...
File test/mjsunit/elements-transition-hoisting.js (right):

http://codereview.chromium.org/9365057/diff/2001/test/mjsunit/elements-transi...
test/mjsunit/elements-transition-hoisting.js:186: print("DUDE");
On 2012/02/15 15:36:48, fschneider wrote:
> Printing can be removed.

Done.

http://codereview.chromium.org/9365057/diff/2001/test/mjsunit/elements-transi...
test/mjsunit/elements-transition-hoisting.js:197: print("DUDE2");
On 2012/02/15 15:36:48, fschneider wrote:
> Printing can be removed.

Done.

[fschneider, 2012-02-16 13:50:51]

https://chromiumcodereview.appspot.com/9365057/diff/12001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/9365057/diff/12001/src/hydrogen.cc#new...
src/hydrogen.cc:1179: if (value->gvn_flags().ContainsAnyOf(depends_flags)) {
You'd also need to handle TransitionElementsKind specially here.

Generally, I think it would be cleaner to treat them specially when doing loop
code motion, rather than here by removing all depends-flags from
TransitionElementsKind.

There is already special code in ProcessLoopBlock for transitions, and this way
you can avoid having code for special treatment in two different places.

[danno, 2012-02-16 14:59:51]

Changed as discussed, please have another look.

[fschneider, 2012-02-16 15:32:28]

LGTM.