Adding Smi support to Add, Sub, Mul, and Bitwise

Adding Smi support to Add, Sub, Mul, and Bitwise

R=svenpanne@chromium.org

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

[Toon Verwaest, 2013-07-24 09:25:08]

PTAL

[Sven Panne, 2013-07-24 11:38:36]

First round of comments.

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

https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3918: if (use_rep.IsNone() || use_rep.IsSmi())
continue;
'if (!use_rep.IsNone() && !use_rep.IsSmi()) return true;' is a bit more
readable, and a predicate on Representation with a good name would be even
better (but I fail to come up with a good one :-).

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

https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3454: if (right()->IsConstant()) return false;
Let's hope that this change doesn't have subtle performance implications...

https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3488: if (!FLAG_smi_binop) {
Nit: I think that

   if (new_rep.IsSmi() && !FLAG_smi_binop) ...

is more readable.

Using a ternary instead of an assignment to a parameter would be nice, too, but
this is a personal preference.

https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:4637: SetFlag(kTruncatingToSmi);
I don't understand this part. Why is this correct? At least a comment is needed.

https://codereview.chromium.org/20070005/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5984: if (elements_kind <= EXTERNAL_SHORT_ELEMENTS)
{
Stuff like this has been there before, but it is ugly as hell: This can't be
understood without seeing the ElementsKind definition (and this even lacks a
comment that the order of enums carries some semantics... :-P

https://codereview.chromium.org/20070005/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/20070005/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:670: return
reinterpret_cast<int32_t>(Smi::FromInt(value));
Evil! ;-)

[Toon Verwaest, 2013-07-24 13:24:01]

Addressed comments, will port to x64 and arm.

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

https://chromiumcodereview.appspot.com/20070005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:3918: if (use_rep.IsNone() || use_rep.IsSmi())
continue;
On 2013/07/24 11:38:36, Sven Panne wrote:
> 'if (!use_rep.IsNone() && !use_rep.IsSmi()) return true;' is a bit more
> readable, and a predicate on Representation with a good name would be even
> better (but I fail to come up with a good one :-).

Done.

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

https://chromiumcodereview.appspot.com/20070005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:3454: if (right()->IsConstant()) return false;
Restored to its original form.

On 2013/07/24 11:38:36, Sven Panne wrote:
> Let's hope that this change doesn't have subtle performance implications...

https://chromiumcodereview.appspot.com/20070005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:3488: if (!FLAG_smi_binop) {
On 2013/07/24 11:38:36, Sven Panne wrote:
> Nit: I think that
> 
>    if (new_rep.IsSmi() && !FLAG_smi_binop) ...
> 
> is more readable.
> 
> Using a ternary instead of an assignment to a parameter would be nice, too,
but
> this is a personal preference.

Done.

https://chromiumcodereview.appspot.com/20070005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:4637: SetFlag(kTruncatingToSmi);
Adjusted to >= 0 for bitand, and added a case for <0 for bitor; and added
comment.

On 2013/07/24 11:38:36, Sven Panne wrote:
> I don't understand this part. Why is this correct? At least a comment is
needed.

https://chromiumcodereview.appspot.com/20070005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:5984: if (elements_kind <= EXTERNAL_SHORT_ELEMENTS)
{
Removed.

On 2013/07/24 11:38:36, Sven Panne wrote:
> Stuff like this has been there before, but it is ugly as hell: This can't be
> understood without seeing the ElementsKind definition (and this even lacks a
> comment that the order of enums carries some semantics... :-P

https://chromiumcodereview.appspot.com/20070005/diff/1/src/ia32/lithium-codeg...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/20070005/diff/1/src/ia32/lithium-codeg...
src/ia32/lithium-codegen-ia32.cc:670: return
reinterpret_cast<int32_t>(Smi::FromInt(value));
I'll see what happens to this after I port it to x64 :)

On 2013/07/24 11:38:36, Sven Panne wrote:
> Evil! ;-)

[Sven Panne, 2013-07-25 06:05:58]

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

https://chromiumcodereview.appspot.com/20070005/diff/6002/src/hydrogen-instru...
src/hydrogen-instructions.h:4645: } else if (op == Token::BIT_OR &&
Can we come up with a case for BIT_XOR? :-)

[Toon Verwaest, 2013-07-25 09:29:24]

Ported to ARM. PTAL again.

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

https://codereview.chromium.org/20070005/diff/6002/src/hydrogen-instructions....
src/hydrogen-instructions.h:4645: } else if (op == Token::BIT_OR &&
I guess not: Even if all the sign-extended bits are the same, the XOR will
depend on the significant bits of the other incoming value.

On 2013/07/25 06:05:58, Sven Panne wrote:
> Can we come up with a case for BIT_XOR? :-)

[Sven Panne, 2013-07-25 11:48:53]

LGTM with a nit

https://codereview.chromium.org/20070005/diff/21001/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/20070005/diff/21001/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:501: int LCodeGen::ToInteger32(LConstantOperand*
op) const {
Hmmm, this was here before, but the return type looks wrong. Shouldn't it be
int32_t? (same for other platforms)

[Toon Verwaest, 2013-07-25 11:51:36]

Addressed comment.

[Toon Verwaest, 2013-07-25 11:53:51]

Committed patchset #8 manually as r15879 (presubmit successful).

[kustermann, 2013-08-07 13:31:18]

I think this CL introduced a bug. See:
https://code.google.com/p/v8/issues/detail?id=2831