Separated smi check from HBoundsCheck.

Separated smi check from HBoundsCheck.

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

[Jakob Kummerow, 2013-02-06 13:43:59]

This is generally the right direction, but I have some suggestions for
simplification.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:818: HBoundsCheck* HBoundsCheck::AddAfter(
I don't see this used anywhere.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:842: HDeoptimizeIfTaggedIsNotSmi* checked_length =
Didn't we agree that HBoundsCheck is only ever used for arrays with FAST
elements, and those always have a Smi as their length? So I don't think we need
this check.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:2973: HInstruction::Verify();
This is the default implementation, no need to override it.

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

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:2616: class HDeoptimizeIfTaggedIsNotSmi: public
HUnaryOperation {
For consistency with HCheckSmi, I'd call this HCheckSmiOrInt32.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:2618: explicit HDeoptimizeIfTaggedIsNotSmi(
no "explicit" necessary (unless you remove the Representation argument, see
below).

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:2626: } else {
I don't think we need this case. This instruction doesn't do anything when the
representation is Integer32, and in the code stubs case it is always Integer32.
So we can simply not insert it at all in that case.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3128: class HGraphBuilder;
Please don't do this. Instead, move the insertion helper function to the graph,
similar to HGraphBuilder::AddSimulate or
HOptimizedGraphBuilder::AddSoftDeoptimize(). (If we need the helper at all --
see my other comments.)

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3165: : key_mode_(key_mode), skip_check_(false) {
nit: more indentation

https://codereview.chromium.org/12208013/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/12208013/diff/1/src/ia32/lithium-ia32.h#newco...
src/ia32/lithium-ia32.h:2279: class LDeoptimizeIfTaggedIsNotSmi: public
LTemplateInstruction<0, 1, 0> {
I think we don't need a new LInstruction.
Instead, I think the "representation.IsTagged() && !type.IsSmi()" logic should
go into the Canonicalization phase, i.e. the HDeoptimizeIfTaggedIsNotSmi should
just remove itself when it is not needed. When it is needed, we can simply
translate it to an LCheckSmi. Or am I missing something?

[Massi, 2013-02-06 19:12:31]

I think I have addressed everything...

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:818: HBoundsCheck* HBoundsCheck::AddAfter(
On 2013/02/06 13:43:59, Jakob wrote:
> I don't see this used anywhere.

Removed.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:842: HDeoptimizeIfTaggedIsNotSmi* checked_length =
On 2013/02/06 13:43:59, Jakob wrote:
> Didn't we agree that HBoundsCheck is only ever used for arrays with FAST
> elements, and those always have a Smi as their length? So I don't think we
need
> this check.

Done.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:2973: HInstruction::Verify();
On 2013/02/06 13:43:59, Jakob wrote:
> This is the default implementation, no need to override it.

The linker complains if I don't override it (and implement it).

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

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:2616: class HDeoptimizeIfTaggedIsNotSmi: public
HUnaryOperation {
On 2013/02/06 13:43:59, Jakob wrote:
> For consistency with HCheckSmi, I'd call this HCheckSmiOrInt32.

Done.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:2618: explicit HDeoptimizeIfTaggedIsNotSmi(
On 2013/02/06 13:43:59, Jakob wrote:
> no "explicit" necessary (unless you remove the Representation argument, see
> below).

Done.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:2626: } else {
On 2013/02/06 13:43:59, Jakob wrote:
> I don't think we need this case. This instruction doesn't do anything when the
> representation is Integer32, and in the code stubs case it is always
Integer32.
> So we can simply not insert it at all in that case.

Done.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3128: class HGraphBuilder;
On 2013/02/06 13:43:59, Jakob wrote:
> Please don't do this. Instead, move the insertion helper function to the
graph,
> similar to HGraphBuilder::AddSimulate or
> HOptimizedGraphBuilder::AddSoftDeoptimize(). (If we need the helper at all --
> see my other comments.)

Done.

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3165: : key_mode_(key_mode), skip_check_(false) {
On 2013/02/06 13:43:59, Jakob wrote:
> nit: more indentation

Done.

https://codereview.chromium.org/12208013/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/12208013/diff/1/src/ia32/lithium-ia32.h#newco...
src/ia32/lithium-ia32.h:2279: class LDeoptimizeIfTaggedIsNotSmi: public
LTemplateInstruction<0, 1, 0> {
On 2013/02/06 13:43:59, Jakob wrote:
> I think we don't need a new LInstruction.
> Instead, I think the "representation.IsTagged() && !type.IsSmi()" logic should
> go into the Canonicalization phase, i.e. the HDeoptimizeIfTaggedIsNotSmi
should
> just remove itself when it is not needed. When it is needed, we can simply
> translate it to an LCheckSmi. Or am I missing something?

Done.

[Jakob Kummerow, 2013-02-07 16:17:14]

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:2973: HInstruction::Verify();
On 2013/02/06 19:12:31, Massi wrote:
> The linker complains if I don't override it (and implement it).

Not for me. Maybe you've forgotten to remove the declaration from the class
definition in hydrogen-instructions.h?

https://codereview.chromium.org/12208013/diff/5001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12208013/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:180: AddInstruction(
Please add a comment here:
// Since we're forcing Integer32 representation for this HBoundsCheck,
// there's no need to Smi-check the index.

https://codereview.chromium.org/12208013/diff/5001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12208013/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2395: HInferRepresentation* h_infer) {
nit: fits on one line now.

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

https://codereview.chromium.org/12208013/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3117: class HGraphBuilder;
still there?

https://codereview.chromium.org/12208013/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3127: HBoundsCheck(HValue* index, HValue* length,
nit: in function/method/ctor definitions (as opposed to calls), each argument
gets its own line (unless everything including the function name fits onto a
single line).

https://codereview.chromium.org/12208013/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3130: : key_mode_(key_mode), skip_check_(false) {
nit: now I want less indentation here :-)
(the ':' gets 4 spaces relative to 'HBoundsCheck(').

https://codereview.chromium.org/12208013/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3155: virtual void PrintDataTo(StringStream*
stream);
I don't see an implementation of this. I guess overriding it is not needed at
all.

https://codereview.chromium.org/12208013/diff/5001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/12208013/diff/5001/src/hydrogen.h#newcode878
src/hydrogen.h:878: HBoundsCheck* AddBoundsCheck(
Since this is not used for stubs, please move it to HOptimizedGraphBuilder.

[Massi, 2013-02-11 10:35:54]

https://chromiumcodereview.appspot.com/12208013/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/12208013/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:2973: HInstruction::Verify();
On 2013/02/07 16:17:14, Jakob wrote:
> On 2013/02/06 19:12:31, Massi wrote:
> > The linker complains if I don't override it (and implement it).
> 
> Not for me. Maybe you've forgotten to remove the declaration from the class
> definition in hydrogen-instructions.h?
> 

I was sure I had deleted, but I did not :-/
Done :-)

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/code-stubs-hydr...
File src/code-stubs-hydrogen.cc (right):

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/code-stubs-hydr...
src/code-stubs-hydrogen.cc:180: AddInstruction(
On 2013/02/07 16:17:14, Jakob wrote:
> Please add a comment here:
> // Since we're forcing Integer32 representation for this HBoundsCheck,
> // there's no need to Smi-check the index.

Done.

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/hydrogen-instru...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/hydrogen-instru...
src/hydrogen-instructions.cc:2395: HInferRepresentation* h_infer) {
On 2013/02/07 16:17:14, Jakob wrote:
> nit: fits on one line now.

Done.

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

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/hydrogen-instru...
src/hydrogen-instructions.h:3117: class HGraphBuilder;
On 2013/02/07 16:17:14, Jakob wrote:
> still there?

Done.

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/hydrogen-instru...
src/hydrogen-instructions.h:3127: HBoundsCheck(HValue* index, HValue* length,
On 2013/02/07 16:17:14, Jakob wrote:
> nit: in function/method/ctor definitions (as opposed to calls), each argument
> gets its own line (unless everything including the function name fits onto a
> single line).

Done.

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/hydrogen-instru...
src/hydrogen-instructions.h:3130: : key_mode_(key_mode), skip_check_(false) {
On 2013/02/07 16:17:14, Jakob wrote:
> nit: now I want less indentation here :-)
> (the ':' gets 4 spaces relative to 'HBoundsCheck(').

Done.

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/hydrogen-instru...
src/hydrogen-instructions.h:3155: virtual void PrintDataTo(StringStream*
stream);
On 2013/02/07 16:17:14, Jakob wrote:
> I don't see an implementation of this. I guess overriding it is not needed at
> all.

HBoundsCheck::PrintDataTo and HBoundsCheck::InferRepresentation are both
implemented.

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/hydrogen.h
File src/hydrogen.h (right):

https://chromiumcodereview.appspot.com/12208013/diff/5001/src/hydrogen.h#newc...
src/hydrogen.h:878: HBoundsCheck* AddBoundsCheck(
On 2013/02/07 16:17:14, Jakob wrote:
> Since this is not used for stubs, please move it to HOptimizedGraphBuilder.

I tried, but it is used in
HGraphBuilder::BuildUncheckedMonomorphicElementAccess.

[Jakob Kummerow, 2013-02-11 10:41:41]

lgtm

[Jakob Kummerow, 2013-02-12 08:52:20]

Patch set 4 LGTM.