Fix IsDeletable() for HStringAdd, HStringCharCodeAt, HStringCharFromCode.

Fix IsDeletable() for HStringAdd, HStringCharCodeAt, HStringCharFromCode.

BUG=
R=mstarzinger@chromium.org, svenpanne@chromium.org

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

[Michael Starzinger, 2013-07-25 14:43:18]

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

https://codereview.chromium.org/20241005/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1154: return !type().IsString() &&
representation().IsTagged();
This predicate looks broken. Shouldn't that be type().IsNumber() here?

https://codereview.chromium.org/20241005/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:6481: virtual bool IsDeletable() const {
nit: This should be private, please move it back down.

https://codereview.chromium.org/20241005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/20241005/diff/1/src/hydrogen.cc#oldcode9081
src/hydrogen.cc:9081: // Fast support for StringAdd.
nit: Can we get the comment back?

[titzer, 2013-07-25 15:31:44]

Jakob, can you confirm or deny that an instruction's representation can change
from Smi, Integer32, or Double to Tagged at any point during crankshafting?

[titzer, 2013-07-25 15:32:55]

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

https://codereview.chromium.org/20241005/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:6481: virtual bool IsDeletable() const {
On 2013/07/25 14:43:18, Michael Starzinger wrote:
> nit: This should be private, please move it back down.

Done.

https://codereview.chromium.org/20241005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/20241005/diff/1/src/hydrogen.cc#oldcode9081
src/hydrogen.cc:9081: // Fast support for StringAdd.
On 2013/07/25 14:43:18, Michael Starzinger wrote:
> nit: Can we get the comment back?

Well, I wanted that one for my code offsets, but I suppose :(

('twas a vestige of trying to delete that method, actually!)

[Jakob Kummerow, 2013-07-25 16:41:08]

On 2013/07/25 15:31:44, titzer wrote:
> Jakob, can you confirm or deny that an instruction's representation can change
> from Smi, Integer32, or Double to Tagged at any point during crankshafting?

I think representations only change during graph building (some special cases
are initialized or frozen there, and instructions with fixed representations set
those in their ctor) and during the InferRepresentations phase (where the vast
majority of representation changes happen). And of course phases that add
instructions later (e.g. some of the bounds check stuff) will likely set a
representation for those new instructions.

[Michael Starzinger, 2013-07-26 08:03:03]

This is an itsy-bitsy fragile, I must say. :D

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

https://codereview.chromium.org/20241005/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:1150: return representation().IsSmiOrInteger32() ||
representation().IsDouble();
Discusssed with Sven: This whole line should be negated.

https://codereview.chromium.org/20241005/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:1152: return !type().IsNonPrimitive();
Discusssed with Sven: What happens for type().IsTagged() here?

https://codereview.chromium.org/20241005/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:1160: return representation().IsSmiOrInteger32() ||
representation().IsDouble();
Likewise.

https://codereview.chromium.org/20241005/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:1162: return !type().IsNonPrimitive();
Likewise.

[Sven Panne, 2013-07-26 08:07:46]

... and the usual plea: Add some tests! :-)

[titzer, 2013-07-29 11:34:55]

On 2013/07/26 08:07:46, Sven Panne wrote:
> ... and the usual plea: Add some tests! :-)

PTAL. I've added tests and simplified the conditions under which these
instructions can be deleted, since it appears that these runtime functions make
several assumptions about their inputs, which results in them being side-effect
free.

[Sven Panne, 2013-07-29 11:48:08]

LGTM with nits.

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

https://codereview.chromium.org/20241005/diff/13001/src/hydrogen-instructions...
src/hydrogen-instructions.h:427: switch (type_) {
The usual pattern within v8 is:

   switch (foo) {
     case BLAH:
     case BLOH:
       return 42;
     case FOO:
       return 271828;
   }
   UNREACHABLE();
   return 0;  // Make -Wall happy

I'll leave it up to you if you change it or not.

https://codereview.chromium.org/20241005/diff/13001/src/hydrogen-instructions...
src/hydrogen-instructions.h:428: case kTaggedPrimitive:  // fallthru
Align.

https://codereview.chromium.org/20241005/diff/13001/test/mjsunit/compiler/dea...
File test/mjsunit/compiler/dead-string-add-warm.js (right):

https://codereview.chromium.org/20241005/diff/13001/test/mjsunit/compiler/dea...
test/mjsunit/compiler/dead-string-add-warm.js:50: assertTrue(dead1(33, 32) ==
33);
assertEquals(33, dead1(33, 32);

Likewise below and in other tests.

[titzer, 2013-07-29 12:26:30]

On 2013/07/29 11:48:08, Sven Panne wrote:
> LGTM with nits.
> 
>
https://codereview.chromium.org/20241005/diff/13001/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/20241005/diff/13001/src/hydrogen-instructions...
> src/hydrogen-instructions.h:427: switch (type_) {
> The usual pattern within v8 is:
> 
>    switch (foo) {
>      case BLAH:
>      case BLOH:
>        return 42;
>      case FOO:
>        return 271828;
>    }
>    UNREACHABLE();
>    return 0;  // Make -Wall happy
> 
> I'll leave it up to you if you change it or not.
> 

In this case, one case does a break and thus falls through to return at the end.

>
https://codereview.chromium.org/20241005/diff/13001/src/hydrogen-instructions...
> src/hydrogen-instructions.h:428: case kTaggedPrimitive:  // fallthru
> Align.

Done.

> 
>
https://codereview.chromium.org/20241005/diff/13001/test/mjsunit/compiler/dea...
> File test/mjsunit/compiler/dead-string-add-warm.js (right):
> 
>
https://codereview.chromium.org/20241005/diff/13001/test/mjsunit/compiler/dea...
> test/mjsunit/compiler/dead-string-add-warm.js:50: assertTrue(dead1(33, 32) ==
> 33);
> assertEquals(33, dead1(33, 32);
> 
> Likewise below and in other tests.

Done.

[Michael Starzinger, 2013-07-29 12:30:03]

LGTM (with one comment).

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

https://codereview.chromium.org/20241005/diff/13001/src/hydrogen-instructions...
src/hydrogen-instructions.h:427: switch (type_) {
On 2013/07/29 11:48:08, Sven Panne wrote:
> The usual pattern within v8 is:
> 
>    switch (foo) {
>      case BLAH:
>      case BLOH:
>        return 42;
>      case FOO:
>        return 271828;
>    }
>    UNREACHABLE();
>    return 0;  // Make -Wall happy
> 
> I'll leave it up to you if you change it or not.

+1 on this comment.

[titzer, 2013-07-29 12:35:53]

Committed patchset #3 manually as r15934 (presubmit successful).