[builtins] Make Math.max and Math.min fast by default.

[builtins] Make Math.max and Math.min fast by default.

The previous versions of Math.max and Math.min made it difficult to
optimize those (that's why we already have custom code in Crankshaft),
and due to lack of ideas what to do about the variable number of
arguments, we will probably need to stick in special code in TurboFan
as well; so inlining those builtins is off the table, hence there's no
real advantage in having them around as "not quite JS" with extra work
necessary in the optimizing compilers to still make those builtins
somewhat fast in cases where we cannot inline them (also there's a
tricky deopt loop in Crankshaft related to Math.min and Math.max, but
that will be dealt with later).

So to sum up: Instead of trying to make Math.max and Math.min semi-fast
in the optimizing compilers with weird work-arounds support %_Arguments
%_ArgumentsLength, we do provide the optimal code as native builtins
instead and call it a day (which gives a nice performance boost on some
benchmarks).

R=jarin@chromium.org

Committed: https://crrev.com/cb9b801069c432e745f0651cf00d65ef88cd7f06
Cr-Commit-Position: refs/heads/master@{#33582}

[Benedikt Meurer, 2016-01-28 12:23:14]

Hey Jaro,

Here's the Math.max and Math.min CL I was cursing about today. :-)
Please take a look.

Thanks,
Benedikt

[Jarin, 2016-01-28 12:39:51]

lgtm. (Although I am not too thrilled about introducing another pile of
platform-specific code.)

Could we also have tests for the ToNumber conversions involved in this?

[Benedikt Meurer, 2016-01-28 12:44:51]

On 2016/01/28 12:39:51, Jarin wrote:
> lgtm. (Although I am not too thrilled about introducing another pile of
> platform-specific code.)

Well this is using variable arguments, so there's no way to currently use a
TurboFan builtin stub.

> Could we also have tests for the ToNumber conversions involved in this?

We have a bunch of them, i.e. mjsunit/to_number_order.js

[Benedikt Meurer, 2016-01-28 12:44:57]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2016-01-28 12:44:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1641083003/#ps20001
(title: "SKIP unrelated ignition failures.")

[commit-bot: I haz the power, 2016-01-28 12:45:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641083003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641083003/20001

[commit-bot: I haz the power, 2016-01-28 13:06:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-28 13:07:19]

Description was changed from

==========
[builtins] Make Math.max and Math.min fast by default.

The previous versions of Math.max and Math.min made it difficult to
optimize those (that's why we already have custom code in Crankshaft),
and due to lack of ideas what to do about the variable number of
arguments, we will probably need to stick in special code in TurboFan
as well; so inlining those builtins is off the table, hence there's no
real advantage in having them around as "not quite JS" with extra work
necessary in the optimizing compilers to still make those builtins
somewhat fast in cases where we cannot inline them (also there's a
tricky deopt loop in Crankshaft related to Math.min and Math.max, but
that will be dealt with later).

So to sum up: Instead of trying to make Math.max and Math.min semi-fast
in the optimizing compilers with weird work-arounds support %_Arguments
%_ArgumentsLength, we do provide the optimal code as native builtins
instead and call it a day (which gives a nice performance boost on some
benchmarks).

R=jarin@chromium.org
==========

to

==========
[builtins] Make Math.max and Math.min fast by default.

The previous versions of Math.max and Math.min made it difficult to
optimize those (that's why we already have custom code in Crankshaft),
and due to lack of ideas what to do about the variable number of
arguments, we will probably need to stick in special code in TurboFan
as well; so inlining those builtins is off the table, hence there's no
real advantage in having them around as "not quite JS" with extra work
necessary in the optimizing compilers to still make those builtins
somewhat fast in cases where we cannot inline them (also there's a
tricky deopt loop in Crankshaft related to Math.min and Math.max, but
that will be dealt with later).

So to sum up: Instead of trying to make Math.max and Math.min semi-fast
in the optimizing compilers with weird work-arounds support %_Arguments
%_ArgumentsLength, we do provide the optimal code as native builtins
instead and call it a day (which gives a nice performance boost on some
benchmarks).

R=jarin@chromium.org

Committed: https://crrev.com/cb9b801069c432e745f0651cf00d65ef88cd7f06
Cr-Commit-Position: refs/heads/master@{#33582}
==========

[commit-bot: I haz the power, 2016-01-28 13:07:20]

Patchset 2 (id:??) landed as
https://crrev.com/cb9b801069c432e745f0651cf00d65ef88cd7f06
Cr-Commit-Position: refs/heads/master@{#33582}

[jbramley, 2016-01-28 18:15:34]

jacob.bramley@arm.com changed reviewers:
+ jacob.bramley@arm.com

[jbramley, 2016-01-28 18:15:35]

ARM64 only. (I'll look at ARM too.)

https://codereview.chromium.org/1641083003/diff/20001/src/arm64/builtins-arm6...
File src/arm64/builtins-arm64.cc (right):

https://codereview.chromium.org/1641083003/diff/20001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:161: __ Mov(x4, x0);
x4 just seems to track how much we need to drop at the end. We drop x4+1 slots,
so if we made this "Add(x4, x0, 1)", we could remove the "Drop(1)" call at the
end.

https://codereview.chromium.org/1641083003/diff/20001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:179: __ JumpIfRoot(x3,
Heap::kHeapNumberMapRootIndex, &convert_number);
Ldr + JumpIfRoot can be JumpIfHeapNumber(x2, &convert_number). (It's the same
code, but a bit easier to read.)

https://codereview.chromium.org/1641083003/diff/20001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:199: __ bind(&done_restore);
Bind

https://codereview.chromium.org/1641083003/diff/20001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:209: __ SmiUntagToDouble(d2, x2);
If a smi argument is the common case, we could untag before the JumpIfSmi and
save a branch. (There's a kSpeculativeUntag option for this purpose.)

https://codereview.chromium.org/1641083003/diff/20001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:215: __ Fcmp(d1, d2);
I think this whole operation could be replaced with one of "Fmin" or "Fmax".
(They don't agree with TurboFan, but I think they do agree with Math.min/max.)

[jbramley, 2016-01-28 18:16:48]

Sorry, I didn't reload and didn't realise it had been submitted. Should I submit
a patch to implement the things above?

[Benedikt Meurer (Google), 2016-01-28 19:50:45]

On 2016/01/28 18:16:48, jbramley wrote:
> Sorry, I didn't reload and didn't realise it had been submitted. Should I
submit
> a patch to implement the things above?

Yes please. I just did a baseline implementation.