Make keyed operations use the unchecked index but still depend on the checked one.

Make keyed operations use the unchecked index but still depend on the checked one.

BUG=

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

[Massi, 2012-12-05 14:28:30]

The end result of this CL is to make array bounds check elimination optional.

I split it in two patches mostly for clarity.

The 1st changes instructions that used the "checked valued" computed by
HBoundsCheck to depended on that value without using it.
This, by itself, "disconnects" the uses so that we will not get artificially
longer live ranges.

The 2nd patch modifies the bounds check elimination pass so that it no longer
performs the "disconnection" because it is not needed anymore.

Everything works if I base this before r13140.
If I apply this CL after r13140 the snapshot creation fails (while hydrogen
compiles the stub).

So there is something wrong either in this CL or in r13140...

[Sven Panne, 2012-12-06 14:59:02]

1st round of comments...

https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:351: return IsInteger32() ? Integer32() : Tagged();
I think we should add an assertion here about the non-Integer32 case (it has to
be Tagged, i guess). I think we rely on the fact that things get monotonically
more specialized.

https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2988: ? Representation::Tagged() :
Formatting nit: Consistently indent the ternary operator like

   condition
       ? trueValue
       : falseValue

if it doesn't fit into a line. I know it's not done consistently in our code,
but we shouldn't make it worse. :-) Alternatively, one might consider using a
switch, which is even more readable IMHO.

https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2989: (index == 3 ? Representation::None() :
Hmmm, I would have expected

 checked_index()->representation().KeyedAccessIndexRequirement()

instead of Representation::None(). If this is really intended, please add a
comment why this is different, it is not obvious when reading the CL.

https://codereview.chromium.org/11445016/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4845: (index == 3 ? Representation::None() :
Representation::Tagged());
See comments regarding indentation/adding a comment above.

https://codereview.chromium.org/11445016/diff/6001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11445016/diff/6001/src/hydrogen.cc#newcode3659
src/hydrogen.cc:3659: if (!FLAG_array_bounds_checks_elimination) return;
For consistency with the other phases, move this to the call site.

[Massi, 2012-12-07 10:11:10]

https://chromiumcodereview.appspot.com/11445016/diff/6001/src/hydrogen-instru...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/11445016/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:351: return IsInteger32() ? Integer32() : Tagged();
On 2012/12/06 14:59:02, Sven Panne wrote:
> I think we should add an assertion here about the non-Integer32 case (it has
to
> be Tagged, i guess). I think we rely on the fact that things get monotonically
> more specialized.

Well, this representation is meant to be used as return value by
RequiredInputRepresentation for the index in keyed loads and stores.
The required representation was int32 and this change is relaxing it to be int32
or tagged.
Generally RequiredInputRepresentation implementations do not assert on anything
about the input, they just state what they require.

https://chromiumcodereview.appspot.com/11445016/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:2988: ? Representation::Tagged() :
On 2012/12/06 14:59:02, Sven Panne wrote:
> Formatting nit: Consistently indent the ternary operator like
> 
>    condition
>        ? trueValue
>        : falseValue
> 
> if it doesn't fit into a line. I know it's not done consistently in our code,
> but we shouldn't make it worse. :-) Alternatively, one might consider using a
> switch, which is even more readable IMHO.

Done.

https://chromiumcodereview.appspot.com/11445016/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:2989: (index == 3 ? Representation::None() :
On 2012/12/06 14:59:02, Sven Panne wrote:
> Hmmm, I would have expected
> 
>  checked_index()->representation().KeyedAccessIndexRequirement()
> 
> instead of Representation::None(). If this is really intended, please add a
> comment why this is different, it is not obvious when reading the CL.

Done.

https://chromiumcodereview.appspot.com/11445016/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:4845: (index == 3 ? Representation::None() :
Representation::Tagged());
On 2012/12/06 14:59:02, Sven Panne wrote:
> See comments regarding indentation/adding a comment above.

Done.

https://chromiumcodereview.appspot.com/11445016/diff/6001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/11445016/diff/6001/src/hydrogen.cc#new...
src/hydrogen.cc:3659: if (!FLAG_array_bounds_checks_elimination) return;
On 2012/12/06 14:59:02, Sven Panne wrote:
> For consistency with the other phases, move this to the call site.

Done.

[Sven Panne, 2012-12-07 10:24:58]

https://chromiumcodereview.appspot.com/11445016/diff/6001/src/hydrogen-instru...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/11445016/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:351: return IsInteger32() ? Integer32() : Tagged();
On 2012/12/07 10:11:11, Massi wrote:
> [...] Generally RequiredInputRepresentation implementations do not assert on
anything
> about the input, they just state what they require.

Well, in general RequiredInputRepresentation doesn't require this cruel hack.
;-) What actually is tried to be modeled is Integer32 \union Tagged (Smi, to be
even more exact), so I'd like to make this hack as explicit as possible. I don't
really care if it's done here or at all call sites, but we need an assertion.

https://chromiumcodereview.appspot.com/11445016/diff/13001/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/11445016/diff/13001/src/hydrogen-instr...
src/hydrogen-instructions.h:2992: default: return Representation::Integer32();
default is evil (a maintenance nightmare), please list the individual cases
explicitly if you keep the switch.

https://chromiumcodereview.appspot.com/11445016/diff/13001/src/hydrogen-instr...
src/hydrogen-instructions.h:4851: default: return Representation::Tagged();
see above

[Sven Panne, 2012-12-07 10:55:00]

LGTM after offline discussion about ASSERTs, if the default cases are removed
from the switches (or contain an UNREACHABLE).