Add DoubleToSmi optimistically, preventing boxing/unboxing of doubles and propagate smi-nessof the …

Add DoubleToSmi optimistically, preventing boxing/unboxing of doubles and propagate smi-nessof the result.
Committed: https://code.google.com/p/dart/source/detail?r=16272

[Vyacheslav Egorov (Google), 2012-12-17 12:53:44]

LGTM

please ensure good test coverage.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:1211: d2i_instr = new
DoubleToIntegerInstr(call->ArgumentAt(0)->value(), call);
It would be great if we could test things like this.

[In V8 we are using "natives syntax" to peak into optimization status:
%GetOptimizationStatus(func) it would be great if we could write tests using
some low level API available only to tests]

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:4060: SetValue(instr, non_constant_);
I think we should stop posponing this to TODOs when implementation is
straightforward.

Our ConstantPropagator is already full of them.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.h:3992: virtual intptr_t ArgumentCount() const
{ return 1; }
I think this should be 0 because pusharguments are dropped.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.h:4023: virtual intptr_t ArgumentCount() const
{ return 1; }
I think this should be zero because this instruction does not contain a call
inside and pushargument is dropped in optimizer.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.h:4030: virtual intptr_t ResultCid() const {
return kSmiCid; }
Comment does not match implementation.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_ia32.cc:2333: __ cvttsd2si(result, value);
This code is almost exactly repeating code in the Intrinsifier::Double_toInt. 

I hope we will find a way to remove this duplication soon.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_x64.cc:2225: __ cvttsd2siq(result, value);
This code does not match the one used in intensifier (uses temp register instead
of comparison with large immediate).

Is there a reason for this? If not please align them.

[srdjan, 2012-12-18 00:12:47]

Added test

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:1211: d2i_instr = new
DoubleToIntegerInstr(call->ArgumentAt(0)->value(), call);
On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> It would be great if we could test things like this.
> 
> [In V8 we are using "natives syntax" to peak into optimization status:
> %GetOptimizationStatus(func) it would be great if we could write tests using
> some low level API available only to tests]

I do manual tests, by tracing deoptimization. I agree it would be nice to have a
framework to better test various optimizations. The worst thing that can happen
is a performance regression.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:4060: SetValue(instr, non_constant_);
On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> I think we should stop posponing this to TODOs when implementation is
> straightforward.
> 
> Our ConstantPropagator is already full of them.

I think there some work needed to implement constant propagation, e.g.. the
input here is an unboxed double typically, not a double object... etc.

I'd rather have those changes in a different CL

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.h:4023: virtual intptr_t ArgumentCount() const
{ return 1; }
On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> I think this should be zero because this instruction does not contain a call
> inside and pushargument is dropped in optimizer.

Done.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.h:4030: virtual intptr_t ResultCid() const {
return kSmiCid; }
On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> Comment does not match implementation.

Removed comment.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_ia32.cc:2333: __ cvttsd2si(result, value);
On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> This code is almost exactly repeating code in the Intrinsifier::Double_toInt. 
> 
> I hope we will find a way to remove this duplication soon.

Code duplication of short patterns is OK. I think it is more readable and
actually easier to maintain, than calling a 'macro' assembler function that does
something.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_x64.cc:2225: __ cvttsd2siq(result, value);
On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> This code does not match the one used in intensifier (uses temp register
instead
> of comparison with large immediate).
> 
> Is there a reason for this? If not please align them.

The code in intrinsifier was not updated to quicker version. Updated
intrinsifier.

[Vyacheslav Egorov (Google), 2012-12-18 11:45:10]

still LGTM.

thanks for adding the test.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:4060: SetValue(instr, non_constant_);
On 2012/12/18 00:12:47, srdjan wrote:
> On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> > I think we should stop posponing this to TODOs when implementation is
> > straightforward.
> > 
> > Our ConstantPropagator is already full of them.
> 
> I think there some work needed to implement constant propagation, e.g.. the
> input here is an unboxed double typically, not a double object... etc.
> 
> I'd rather have those changes in a different CL

Agreed about separate CL.

I was under impression that we run constant propagation before representation
selection but apparently this is not the case. I am not sure why. As it is know
there is a bit of "obstruction" inserted between double constants and their
uses.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.h:4023: virtual intptr_t ArgumentCount() const
{ return 1; }
On 2012/12/18 00:12:47, srdjan wrote:
> On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> > I think this should be zero because this instruction does not contain a call
> > inside and pushargument is dropped in optimizer.
> 
> Done.

Actually Definition has a default implementation that returns 0 so you can just
remove it.

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_ia32.cc:2333: __ cvttsd2si(result, value);
On 2012/12/18 00:12:47, srdjan wrote:
> On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> > This code is almost exactly repeating code in the
Intrinsifier::Double_toInt. 
> > 
> > I hope we will find a way to remove this duplication soon.
> 
> Code duplication of short patterns is OK. I think it is more readable and
> actually easier to maintain, than calling a 'macro' assembler function that
does
> something.

Well, the discrepancy in x64 demonstrates that it is not easier to maintain at
all. However at the moment I see no easy alternative.

https://codereview.chromium.org/11568044/diff/19002/runtime/vm/intrinsifier_x...
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/11568044/diff/19002/runtime/vm/intrinsifier_x...
runtime/vm/intrinsifier_x64.cc:1395: __ movq(TMP, RAX);
I thought only Assembler could use TMP?

[srdjan, 2012-12-18 16:29:47]

Thanks!

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/11568044/diff/6008/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.h:4023: virtual intptr_t ArgumentCount() const
{ return 1; }
On 2012/12/18 11:45:10, Vyacheslav Egorov (Google) wrote:
> On 2012/12/18 00:12:47, srdjan wrote:
> > On 2012/12/17 12:53:44, Vyacheslav Egorov (Google) wrote:
> > > I think this should be zero because this instruction does not contain a
call
> > > inside and pushargument is dropped in optimizer.
> > 
> > Done.
> 
> Actually Definition has a default implementation that returns 0 so you can
just
> remove it.

Done, thanks.

https://codereview.chromium.org/11568044/diff/19002/runtime/vm/intrinsifier_x...
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/11568044/diff/19002/runtime/vm/intrinsifier_x...
runtime/vm/intrinsifier_x64.cc:1395: __ movq(TMP, RAX);
On 2012/12/18 11:45:10, Vyacheslav Egorov (Google) wrote:
> I thought only Assembler could use TMP?

You are correct. Switched to using RCX instead.