Add graph intrinsics for math library functions

Add graph intrinsics for many math library functions

- sin, cos, tan, asin, acos, atan, and atan2.

R=fschneider@google.com

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

[Cutch, 2016-03-15 22:16:35]

Description was changed from

==========
Add graph intrinsics for math library functions

BUG=
==========

to

==========
Add graph intrinsics for math library functions

- Work in progress
==========

[Cutch, 2016-03-15 22:16:35]

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

[Cutch, 2016-03-15 22:17:05]

PTAL before I do more intrinsics.

[Florian Schneider, 2016-03-16 15:51:49]

https://codereview.chromium.org/1808503003/diff/1/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/1808503003/diff/1/runtime/vm/intrinsifier.cc#...
runtime/vm/intrinsifier.cc:307: AddInstruction(new CheckClassInstr(new
Value(boxed),
Since these math functions call toDouble on their inputs, CheckClass may not be
needed, because UnboxInstr checks its input for double/smi-ness, and deopts for
other cids.

[Cutch, 2016-03-16 19:35:56]

https://codereview.chromium.org/1808503003/diff/1/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/1808503003/diff/1/runtime/vm/intrinsifier.cc#...
runtime/vm/intrinsifier.cc:307: AddInstruction(new CheckClassInstr(new
Value(boxed),
On 2016/03/16 15:51:49, Florian Schneider wrote:
> Since these math functions call toDouble on their inputs, CheckClass may not
be
> needed, because UnboxInstr checks its input for double/smi-ness, and deopts
for
> other cids.

We are recognizing 'sin' and not '_sin', so the argument has not been converted
to a double before hand.

[Cutch, 2016-03-16 20:09:03]

Description was changed from

==========
Add graph intrinsics for math library functions

- Work in progress
==========

to

==========
Add graph intrinsics for many math library functions

- sin, cos, tan, asin, acos, atan, and atan2.
==========

[Cutch, 2016-03-16 20:10:37]

PTAL

[Cutch, 2016-03-17 22:18:56]

On 2016/03/16 20:10:37, Cutch wrote:
> PTAL

PTAL

[Florian Schneider, 2016-03-19 09:40:56]

The only comment is about using UnboxInstr only, and avoiding the CheckClass.

Otherwise lgtm.

https://codereview.chromium.org/1808503003/diff/1/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/1808503003/diff/1/runtime/vm/intrinsifier.cc#...
runtime/vm/intrinsifier.cc:307: AddInstruction(new CheckClassInstr(new
Value(boxed),
On 2016/03/16 19:35:55, Cutch wrote:
> On 2016/03/16 15:51:49, Florian Schneider wrote:
> > Since these math functions call toDouble on their inputs, CheckClass may not
> be
> > needed, because UnboxInstr checks its input for double/smi-ness, and deopts
> for
> > other cids.
> 
> We are recognizing 'sin' and not '_sin', so the argument has not been
converted
> to a double before hand.

Yes, this is why I think just UnboxInstr would just do the right thing:

patch double sin(num value) => _sin(value.toDouble()); 

converts the input to double, non-double, non-smi inputs would deopts (or go to
slow-path in case of intrinsic code)

[Florian Schneider, 2016-03-21 16:40:15]

One more general comment: The graph intrinsics allow a quick way to avoid the
overhead of calling a native, but we still have unboxing/boxing overhead.

Let's go with this and measure the speedup. If it is not good enough, we should
try to eliminate the overhead of unbox/box operations (assuming that the
inputs/result of Math functions are used in other double operations)

[Cutch, 2016-03-21 17:03:48]

https://codereview.chromium.org/1808503003/diff/1/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/1808503003/diff/1/runtime/vm/intrinsifier.cc#...
runtime/vm/intrinsifier.cc:307: AddInstruction(new CheckClassInstr(new
Value(boxed),
On 2016/03/19 09:40:56, Florian Schneider wrote:
> On 2016/03/16 19:35:55, Cutch wrote:
> > On 2016/03/16 15:51:49, Florian Schneider wrote:
> > > Since these math functions call toDouble on their inputs, CheckClass may
not
> > be
> > > needed, because UnboxInstr checks its input for double/smi-ness, and
deopts
> > for
> > > other cids.
> > 
> > We are recognizing 'sin' and not '_sin', so the argument has not been
> converted
> > to a double before hand.
> 
> Yes, this is why I think just UnboxInstr would just do the right thing:
> 
> patch double sin(num value) => _sin(value.toDouble()); 
> 
> converts the input to double, non-double, non-smi inputs would deopts (or go
to
> slow-path in case of intrinsic code)
> 

Done.

[Cutch, 2016-03-21 17:23:54]

Description was changed from

==========
Add graph intrinsics for many math library functions

- sin, cos, tan, asin, acos, atan, and atan2.
==========

to

==========
Add graph intrinsics for many math library functions

- sin, cos, tan, asin, acos, atan, and atan2.

R=fschneider@google.com

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

[Cutch, 2016-03-21 17:23:55]

Committed patchset #3 (id:40001) manually as
7dc6bdefa3f0c0078933a3c1ea80fb8c4c3696d1 (presubmit successful).

[Cutch, 2016-03-21 19:46:21]

Florian,

The CheckClass instr you had me remove ended up causing many test failures on
the bots[1]. As I explained previously, the call to '.toDouble()' occurs *after*
the intrinsic executes and not before. Inserting the CheckClass instruction as
in my original CL results in all tests passing.

I've re-uploaded my CL here: https://codereview.chromium.org/1821783002

[1]
https://build.chromium.org/p/client.dart/builders/vm-mac-debug-x64-be/builds/...

[Florian Schneider, 2016-03-22 08:56:28]

On 2016/03/21 19:46:21, Cutch wrote:
> Florian,
> 
> The CheckClass instr you had me remove ended up causing many test failures on
> the bots[1]. As I explained previously, the call to '.toDouble()' occurs
*after*
> the intrinsic executes and not before. Inserting the CheckClass instruction as
> in my original CL results in all tests passing.
> 
> I've re-uploaded my CL here: https://codereview.chromium.org/1821783002
> 
> [1]
>
https://build.chromium.org/p/client.dart/builders/vm-mac-debug-x64-be/builds/...

No, we intrinsify atan2 (and not the native _atan2) - the toDouble occurs in the
slow-path. The reason for the failures is that AddUnboxInstr assumes that the
input is checked: It sets the reaching type of the input operand to kDoubleCid.
As a result, there is not class check emitted as part of the UnboxInstr.

Your solution works by adding the explicit CheckClass before, but does not
handle the smi-conversion case.

I think the best solution would be to add a UnboxInstr where the input value has
unknown compile-type.