Inline toDouble calls on mints

* Fixes ICData collection on Double.fromInteger factory constructor (for both smi and mint).
* Inlines instance calls to toDouble on mint.
* Adds fast path mint -> double for IA32.

R=fschneider@google.com

Committed: https://code.google.com/p/dart/source/detail?r=40501

[Cutch, 2014-09-16 16:01:53]

johnmccutchan@google.com changed reviewers:
+ fschneider@google.com

[Cutch, 2014-09-16 16:04:11]

PTAL while I work on ARM backend.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/flow_graph_optimi...
File runtime/vm/flow_graph_optimizer.cc (left):

https://codereview.chromium.org/562203005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:4506: if (call->HasICData() &&
(call->ic_data()->NumberOfChecks() == 1)) {
This code had two problems:

1) DoubleFromInteger is a factory constructor and we were checking argument 0
(which is the type and is always null) so this never activated.
2) toDouble calls the factory constructor but toDouble on smis was inlined as an
instance call. So we never even hit here.

[Florian Schneider, 2014-09-17 11:15:33]

https://codereview.chromium.org/562203005/diff/1/runtime/vm/assembler_ia32_te...
File runtime/vm/assembler_ia32_test.cc (right):

https://codereview.chromium.org/562203005/diff/1/runtime/vm/assembler_ia32_te...
runtime/vm/assembler_ia32_test.cc:2066: EXPECT_FLOAT_EQ(6.0, res, 0.001);
I think can be 6.0 represented exactly. No need for 0.001 tolerance.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/assembler_ia32_te...
runtime/vm/assembler_ia32_test.cc:2086: EXPECT_FLOAT_EQ(-6.0, res, 0.001);
I think can be -6.0 represented exactly. No need for 0.001 tolerance.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/flow_graph_optimi...
File runtime/vm/flow_graph_optimizer.cc (left):

https://codereview.chromium.org/562203005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:4506: if (call->HasICData() &&
(call->ic_data()->NumberOfChecks() == 1)) {
On 2014/09/16 16:04:11, Cutch wrote:
> This code had two problems:
> 
> 1) DoubleFromInteger is a factory constructor and we were checking argument 0
> (which is the type and is always null) so this never activated.
> 2) toDouble calls the factory constructor but toDouble on smis was inlined as
an
> instance call. So we never even hit here.

Good catch. Double.fromInteger is called in a lot of other places (e.g.
Double._addFromInteger), so my suggestion instead of removing this code is to
fix it by making num_checked_arguments 2 for calls to Double.fromInteger and
check the second argument using ArgIsAlways(kSmiCid, ic_data, 1).

Then you can the case for mint here as well.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.cc (left):

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:2633: num_args_checked = 1;
Change this to 2 to make inlining of DoubleFromInteger work as expected.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:7185: virtual intptr_t ArgumentCount() const
{ return 1; }
Why is ArgumentCount 1? There should not be any pushed arguments for this
instructions.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:7264: virtual intptr_t ArgumentCount() const
{ return 1; }
Why is ArgumentCount 1? There should not be any pushed arguments for this
instructions.

[Cutch, 2014-09-18 16:39:49]

https://codereview.chromium.org/562203005/diff/1/runtime/vm/assembler_ia32_te...
File runtime/vm/assembler_ia32_test.cc (right):

https://codereview.chromium.org/562203005/diff/1/runtime/vm/assembler_ia32_te...
runtime/vm/assembler_ia32_test.cc:2066: EXPECT_FLOAT_EQ(6.0, res, 0.001);
On 2014/09/17 11:15:32, Florian Schneider wrote:
> I think can be 6.0 represented exactly. No need for 0.001 tolerance.

Done.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/assembler_ia32_te...
runtime/vm/assembler_ia32_test.cc:2086: EXPECT_FLOAT_EQ(-6.0, res, 0.001);
On 2014/09/17 11:15:32, Florian Schneider wrote:
> I think can be -6.0 represented exactly. No need for 0.001 tolerance.

Done.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/flow_graph_optimi...
File runtime/vm/flow_graph_optimizer.cc (left):

https://codereview.chromium.org/562203005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:4506: if (call->HasICData() &&
(call->ic_data()->NumberOfChecks() == 1)) {
On 2014/09/17 11:15:32, Florian Schneider wrote:
> On 2014/09/16 16:04:11, Cutch wrote:
> > This code had two problems:
> > 
> > 1) DoubleFromInteger is a factory constructor and we were checking argument
0
> > (which is the type and is always null) so this never activated.
> > 2) toDouble calls the factory constructor but toDouble on smis was inlined
as
> an
> > instance call. So we never even hit here.
> 
> Good catch. Double.fromInteger is called in a lot of other places (e.g.
> Double._addFromInteger), so my suggestion instead of removing this code is to
> fix it by making num_checked_arguments 2 for calls to Double.fromInteger and
> check the second argument using ArgIsAlways(kSmiCid, ic_data, 1).
> 
> Then you can the case for mint here as well.

Done.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.cc (left):

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:2633: num_args_checked = 1;
On 2014/09/17 11:15:32, Florian Schneider wrote:
> Change this to 2 to make inlining of DoubleFromInteger work as expected.

Done.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:7185: virtual intptr_t ArgumentCount() const
{ return 1; }
On 2014/09/17 11:15:32, Florian Schneider wrote:
> Why is ArgumentCount 1? There should not be any pushed arguments for this
> instructions.

Fixed here.

https://codereview.chromium.org/562203005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:7264: virtual intptr_t ArgumentCount() const
{ return 1; }
On 2014/09/17 11:15:32, Florian Schneider wrote:
> Why is ArgumentCount 1? There should not be any pushed arguments for this
> instructions.

And here.

[Florian Schneider, 2014-09-19 09:49:01]

lgtm

https://codereview.chromium.org/562203005/diff/60001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/562203005/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:69: #else
I think there should be a TODO for the other platforms that have unboxed mints.

[Cutch, 2014-09-19 16:17:41]

https://codereview.chromium.org/562203005/diff/60001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/562203005/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:69: #else
On 2014/09/19 09:49:00, Florian Schneider wrote:
> I think there should be a TODO for the other platforms that have unboxed
mints.

Added a note about ARM and a TODO to check for MIPS.

[Cutch, 2014-09-19 16:23:48]

Committed patchset #5 (id:80001) manually as 40501 (presubmit successful).