Add smi support to all binops minus shr, sar, shl, div and mod.

Add smi support to all binops minus shr, sar, shl, div and mod.

[Toon Verwaest, 2013-06-04 09:36:40]

PTAL. Only ia32 for now.

[Sven Panne, 2013-06-04 09:53:55]

DBC below. Somehow it is ugly that the notion of "representation" is leaking
into range analysis, which is fundamentally wrong. But this has been there
(partly) before and is a story for a different CL IMHO...

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

https://chromiumcodereview.appspot.com/16206004/diff/2001/src/hydrogen-instru...
src/hydrogen-instructions.cc:1852: (r.IsSmi() && !a->Includes(Smi::kMinValue))
||
* Re-order back to a, then b test.
* Introduce 'min_value = r.IsSmi() ? Smi::kMinValue() : kMinInt' and use that.

Same elsewhere.

[Sven Panne, 2013-06-04 10:01:07]

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

https://chromiumcodereview.appspot.com/16206004/diff/2001/src/hydrogen-instru...
src/hydrogen-instructions.cc:1852: (r.IsSmi() && !a->Includes(Smi::kMinValue))
||
On 2013/06/04 09:53:56, Sven Panne wrote:
> * Re-order back to a, then b test.
> * Introduce 'min_value = r.IsSmi() ? Smi::kMinValue() : kMinInt' and use that.
> 
> Same elsewhere.

Thinking about it: We have to avoid an idiv of 0x80000000 by 0x7fffffff, how can
this happen in the Smi case here?

Anyway, this flag setting business does not belong here at all, anyway, the code
generation should look at the ranges IMHO.

[Toon Verwaest, 2013-06-04 15:06:40]

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

https://chromiumcodereview.appspot.com/16206004/diff/2001/src/hydrogen-instru...
src/hydrogen-instructions.cc:1852: (r.IsSmi() && !a->Includes(Smi::kMinValue))
||
On 2013/06/04 10:01:07, Sven Panne wrote:
> On 2013/06/04 09:53:56, Sven Panne wrote:
> > * Re-order back to a, then b test.
> > * Introduce 'min_value = r.IsSmi() ? Smi::kMinValue() : kMinInt' and use
that.
> > 
> > Same elsewhere.
> 
> Thinking about it: We have to avoid an idiv of 0x80000000 by 0x7fffffff, how
can
> this happen in the Smi case here?
> 
> Anyway, this flag setting business does not belong here at all, anyway, the
code
> generation should look at the ranges IMHO.

You are right. I actually (temporarily) backed out changes for HMod; so I should
probably also remove this code again.

[danno, 2013-06-06 12:32:13]

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

https://codereview.chromium.org/16206004/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3380: if (r.IsSmi() && CannotBothBeSmi()) {
A comment might be nice.

https://codereview.chromium.org/16206004/diff/2001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16206004/diff/2001/src/hydrogen.cc#newcode3475
src/hydrogen.cc:3475: !from.Equals(change->to())));
Why did you add the Equals() clause? This should already be guaranteed. Is there
extra value in the check here?

https://codereview.chromium.org/16206004/diff/2001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/16206004/diff/2001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:592: HConstant* constant =
chunk_->LookupConstant(op);
why do you need to pass in the representation? Can't you get this from
op->hydrogen()->representation()?