Add DoubleTestOp instruction

Add DoubleTestOp instruction

This gives a ~5% bump to the Flutter layout benchmark
in checked mode.

R=fschneider@google.com

Committed: https://github.com/dart-lang/sdk/commit/fa855cf1c2794bf60e3516aae8717a6ce65ee47e

[zra, 2016-10-18 15:51:25]

Description was changed from

==========
DBC: Add DoubleIsNaN instruction.
==========

to

==========
Add DoubleTestOp instruction

This gives a ~5% bump to the Flutter layout benchmark
in checked mode.
==========

[zra, 2016-10-18 15:52:00]

zra@google.com changed reviewers:
+ fschneider@google.com, rmacnak@google.com

[Florian Schneider, 2016-10-18 16:03:23]

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:5347: class DoubleTestOpInstr : public
TemplateDefinition<1, NoThrow, Pure> {
Can you make this inherit from ComparisonInstr?

This would allow even better code for branches like if (x.isNaN) {...}

It should work like for TestCidsInstr (which also takes only one input), where
we override InputCount() to return 1.

There is a TODO there to make ComparisonInstr more general, and allow 1 or 2
inputs.

[zra, 2016-10-18 17:20:40]

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:5347: class DoubleTestOpInstr : public
TemplateDefinition<1, NoThrow, Pure> {
On 2016/10/18 16:03:23, Florian Schneider wrote:
> Can you make this inherit from ComparisonInstr?
> 
> This would allow even better code for branches like if (x.isNaN) {...}
> 
> It should work like for TestCidsInstr (which also takes only one input), where
> we override InputCount() to return 1.
> 
> There is a TODO there to make ComparisonInstr more general, and allow 1 or 2
> inputs.

Are there any other cases where we use an inlined getter for a comparison that I
could crib from? Otherwise, I think I'll probably punt on this for now.

[Florian Schneider, 2016-10-18 20:34:21]

Lgtm.

Handling get:isNegative may be done in a separate CL as well. We have intrinsic
code already for it, so just the intermediate_language_arch parts would be
needed to support it the same way.

https://codereview.chromium.org/2423843002/diff/100001/runtime/lib/double.dart
File runtime/lib/double.dart (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/lib/double.dar...
runtime/lib/double.dart:95: bool get isInfinite => _isInfinite;
Why must these be private getters?

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_inliner.cc:3680: case MethodRecognizer::kDoubleIsNaN:
Handle DoubleIsNegative?

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:5347: class DoubleTestOpInstr : public
TemplateDefinition<1, NoThrow, Pure> {
On 2016/10/18 17:20:40, zra wrote:
> On 2016/10/18 16:03:23, Florian Schneider wrote:
> > Can you make this inherit from ComparisonInstr?
> > 
> > This would allow even better code for branches like if (x.isNaN) {...}
> > 
> > It should work like for TestCidsInstr (which also takes only one input),
where
> > we override InputCount() to return 1.
> > 
> > There is a TODO there to make ComparisonInstr more general, and allow 1 or 2
> > inputs.
> 
> Are there any other cases where we use an inlined getter for a comparison that
I
> could crib from? Otherwise, I think I'll probably punt on this for now.

I'm not aware of any. It should be straight-forward following the pattern used
for TestCidsInstr. Basically, you only need to add a EmitBranchCode method,
which branches to true- or false-label directly.

I'm ok with doing this as a separate CL.

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_ia32.cc:3928: __ AddImmediate(ESP,
Immediate(-8));
s/-8/-kDoubleSize/

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_mips.cc:4584: // base == -Infinity -> call pow;
The comment seems out of sync.

s/-Infinity/Infinity/

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_x64.cc:3789: __ AddImmediate(RSP,
Immediate(-8));
-kDoubleSize

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/method_reco...
File runtime/vm/method_recognizer.h (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/method_reco...
runtime/vm/method_recognizer.h:49: V(_Double, get:_isNaN, DoubleIsNaN, Bool,
0x09193f42)                        \
You can omit the method here, if it is in the list below. Then, I think you
don't need to add the private getters, and can leave double.dart unchanged.

[zra, 2016-10-18 22:36:33]

https://codereview.chromium.org/2423843002/diff/100001/runtime/lib/double.dart
File runtime/lib/double.dart (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/lib/double.dar...
runtime/lib/double.dart:95: bool get isInfinite => _isInfinite;
On 2016/10/18 20:34:20, Florian Schneider wrote:
> Why must these be private getters?

Done.

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_inliner.cc:3680: case MethodRecognizer::kDoubleIsNaN:
On 2016/10/18 20:34:20, Florian Schneider wrote:
> Handle DoubleIsNegative?

Acknowledged.

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:5347: class DoubleTestOpInstr : public
TemplateDefinition<1, NoThrow, Pure> {
On 2016/10/18 20:34:20, Florian Schneider wrote:
> On 2016/10/18 17:20:40, zra wrote:
> > On 2016/10/18 16:03:23, Florian Schneider wrote:
> > > Can you make this inherit from ComparisonInstr?
> > > 
> > > This would allow even better code for branches like if (x.isNaN) {...}
> > > 
> > > It should work like for TestCidsInstr (which also takes only one input),
> where
> > > we override InputCount() to return 1.
> > > 
> > > There is a TODO there to make ComparisonInstr more general, and allow 1 or
2
> > > inputs.
> > 
> > Are there any other cases where we use an inlined getter for a comparison
that
> I
> > could crib from? Otherwise, I think I'll probably punt on this for now.
> 
> I'm not aware of any. It should be straight-forward following the pattern used
> for TestCidsInstr. Basically, you only need to add a EmitBranchCode method,
> which branches to true- or false-label directly.

Does the inliner magically know that it can take a ComparisonInstr and use its
EmitBranchCode directly? Or is there extra wiring-up that needs to be done?

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_ia32.cc:3928: __ AddImmediate(ESP,
Immediate(-8));
On 2016/10/18 20:34:20, Florian Schneider wrote:
> s/-8/-kDoubleSize/

Done.

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_mips.cc:4584: // base == -Infinity -> call pow;
On 2016/10/18 20:34:20, Florian Schneider wrote:
> The comment seems out of sync.
> 
> s/-Infinity/Infinity/

Done.

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_x64.cc:3789: __ AddImmediate(RSP,
Immediate(-8));
On 2016/10/18 20:34:21, Florian Schneider wrote:
> -kDoubleSize

Done.

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/method_reco...
File runtime/vm/method_recognizer.h (right):

https://codereview.chromium.org/2423843002/diff/100001/runtime/vm/method_reco...
runtime/vm/method_recognizer.h:49: V(_Double, get:_isNaN, DoubleIsNaN, Bool,
0x09193f42)                        \
On 2016/10/18 20:34:21, Florian Schneider wrote:
> You can omit the method here, if it is in the list below. Then, I think you
> don't need to add the private getters, and can leave double.dart unchanged.

Done.

[zra, 2016-10-19 04:24:15]

Description was changed from

==========
Add DoubleTestOp instruction

This gives a ~5% bump to the Flutter layout benchmark
in checked mode.
==========

to

==========
Add DoubleTestOp instruction

This gives a ~5% bump to the Flutter layout benchmark
in checked mode.

R=fschneider@google.com

Committed:
https://github.com/dart-lang/sdk/commit/fa855cf1c2794bf60e3516aae8717a6ce65ee47e
==========

[zra, 2016-10-19 04:24:16]

Committed patchset #7 (id:120001) manually as
fa855cf1c2794bf60e3516aae8717a6ce65ee47e (presubmit successful).