Add fake data dependencies (instead of disabling GVN) to fix code motion

Add fake data dependencies (instead of disabling GVN) to fix code motion

for HInstructions used in polymorphic array access code.

TEST=existing tests pass.

Committed: http://code.google.com/p/v8/source/detail?r=8699

[Jakob Kummerow, 2011-07-01 11:55:48]

PTAL. 
I haven't had time yet to manually inspect the machine code that gets generated
with this patch, will do that next week. Tests pass, so it should be generally
OK.

[Jakob Kummerow, 2011-07-08 11:49:20]

Looking at the generated machine code I found that some type-dependent
instructions were still hoisted out of loops. While it didn't make any tests
fail, preventing these possibly illegal memory accesses seems safer.

PTAL.

[Kevin Millikin (Chromium), 2011-07-11 15:03:53]

LGTM.

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2401: explicit HJSArrayLength(HValue* value, HValue*
typecheck)
No need for explicit.

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2409: SetFlag(kDependsOnMaps);
I'm not positive we need this flag anymore, because it depends only on the map
that it's checking.

Everything should be made to work if we remove this flag.  That would enable
moving/eliminating this instruction in the case that some unrelated map has
changed.

The code as written is safe, so leave it as is and we can talk about changing it
when everyone gets back from vacation.

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:3438: explicit HLoadElements(HValue* value, HValue*
typecheck)
Same, no need for explicit.

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen.cc#newcode3267
src/hydrogen.cc:3267: elements = new(zone()) HLoadElements(literal, literal);
The second occurrence of literal is used as a token to indicate that you've
discharged your obligation to check this map ("trust me, I know what I'm
doing").  I wonder if the undefined value or something is better to use here?

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen.cc#newcode3983
src/hydrogen.cc:3983: new(zone()) HLoadElements(object, elements_kind_branch));
Should we assert the check instruction is not NULL here?

[Jakob Kummerow, 2011-07-19 14:55:43]

I finally got around to rebasing this CL onto current trunk and addressing
Kevin's comments. It turned out that executing HLoadElements on any array no
longer seems to be an issue (at least I've been unable to reproduce any
problem); so I could change that part of the CL to allow hoisting of
HLoadElements (rather than preventing it by different means than before).

Florian: as I changed quite a bit, and Kevin is on vacation, could you take a
quick look to see if this is all sane?

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2401: explicit HJSArrayLength(HValue* value, HValue*
typecheck)
On 2011/07/11 15:03:54, Kevin Millikin wrote:
> No need for explicit.

Done.

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:3438: explicit HLoadElements(HValue* value, HValue*
typecheck)
On 2011/07/11 15:03:54, Kevin Millikin wrote:
> Same, no need for explicit.

Obsolete (I undid the changes to HLoadElements).

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen.cc#newcode3267
src/hydrogen.cc:3267: elements = new(zone()) HLoadElements(literal, literal);
On 2011/07/11 15:03:54, Kevin Millikin wrote:
> The second occurrence of literal is used as a token to indicate that you've
> discharged your obligation to check this map ("trust me, I know what I'm
> doing").  I wonder if the undefined value or something is better to use here?

Obsolete (I undid the changes to HLoadElements).

http://codereview.chromium.org/7298003/diff/3001/src/hydrogen.cc#newcode3983
src/hydrogen.cc:3983: new(zone()) HLoadElements(object, elements_kind_branch));
On 2011/07/11 15:03:54, Kevin Millikin wrote:
> Should we assert the check instruction is not NULL here?

Good catch! Depending on the JS code being executed, the check instruction
actually could be NULL at this point. While investigating, I found that hoisting
prevention for HLoadElements no longer seems to be necessary, so I removed it.

[fschneider, 2011-07-20 11:24:37]

LGTM.

This is the right direction. We could do the same for the bounds-check and
others. I'd assume that the cost of additional HValue-operands is not noticable.