ARM64: Generate optimized code for Math.floor and Math.round with double outputs.

ARM64: Generate optimized code for Math.floor and Math.round with double outputs.

R=jkummerow@chromium.org, ulan@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21091

[ulan, 2014-04-25 13:40:54]

The change looks good. Adding Jakob for hydrogen parts.

Do existing tests exercise the new code?

[Jakob Kummerow, 2014-04-25 14:12:03]

Only looked at hydrogen changes.

https://codereview.chromium.org/258793002/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/258793002/diff/1/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:4211: if (!rep_required) {
I'm not sure I like the way you're prioritizing required over observed
representations. If any of the uses (ideally, including transitive uses through
Phis, but that's clearly outside the scope of this CL) has observed doubles,
that should always be enough of a reason to choose double representation.

Example:
function f(x) { return Math.round(x) + "."; }
f(0x80000000);
f(0x80000000);
%OptimizeFunctionOnNextCall(f);
f(0x80000000);

This will deopt with the current patch, because the addition requires tagged
inputs, so the Round ignores it and chooses int32 representation. If you honor
the observed double feedback, you can avoid the deopt.
I think you'll just have to look at both required and observed representations.

https://codereview.chromium.org/258793002/diff/1/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:4224: break;
nit: indentation. Also, a comment would be nice (e.g. "Having seen one double
use is enough."), because changing control flow based on a tracing flag looks
quite surprising at first.

[Alexandre Rames, 2014-04-25 16:05:26]

I will update the patch to address your comments, and update the round and floor
testing for the new paths.

https://codereview.chromium.org/258793002/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/258793002/diff/1/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:4211: if (!rep_required) {
Removed consideration of the required representation.

On 2014/04/25 14:12:03, Jakob wrote:
> I'm not sure I like the way you're prioritizing required over observed
> representations. If any of the uses (ideally, including transitive uses
through
> Phis, but that's clearly outside the scope of this CL) has observed doubles,
> that should always be enough of a reason to choose double representation.
> 
> Example:
> function f(x) { return Math.round(x) + "."; }
> f(0x80000000);
> f(0x80000000);
> %OptimizeFunctionOnNextCall(f);
> f(0x80000000);
> 
> This will deopt with the current patch, because the addition requires tagged
> inputs, so the Round ignores it and chooses int32 representation. If you honor
> the observed double feedback, you can avoid the deopt.
> I think you'll just have to look at both required and observed
representations.

https://codereview.chromium.org/258793002/diff/1/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:4224: break;
On 2014/04/25 14:12:03, Jakob wrote:
> nit: indentation. Also, a comment would be nice (e.g. "Having seen one double
> use is enough."), because changing control flow based on a tracing flag looks
> quite surprising at first.

Done.

[Alexandre Rames, 2014-04-28 12:38:03]

Just uploaded an updated patch.
I have extended the existing testing to also test the 'double output' path.

[Jakob Kummerow, 2014-04-28 13:53:31]

https://codereview.chromium.org/258793002/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/258793002/diff/1/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:4211: if (!rep_required) {
On 2014/04/25 16:05:26, Alexandre Rames wrote:
> Removed consideration of the required representation.

Any reason you're not looking at both required and observed representations as I
suggested? If the only use is e.g. another unary math function (which requires
double inputs and, being a call, doesn't have any observed representation), it
would be nice to avoid deopts too. (If in my previous example you use "function
f(x) { return Math.log(Math.round(x)); }" that should exhibit what I mean.)

[Alexandre Rames, 2014-04-28 15:47:57]

On 2014/04/28 13:53:31, Jakob wrote:

> Any reason you're not looking at both required and observed representations as
I
> suggested? If the only use is e.g. another unary math function (which requires
> double inputs and, being a call, doesn't have any observed representation), it
> would be nice to avoid deopts too. (If in my previous example you use
"function
> f(x) { return Math.log(Math.round(x)); }" that should exhibit what I mean.)

I had misunderstood your first comment. Updated the code to use both oberved and
required representations, but prefer the observed representation (fixes the
`Math.log(Math.round())` test-case) and also use a double output representation
for tagged inputs (fixes the `Math.round(x) + "."` test-case).

[Jakob Kummerow, 2014-04-28 17:10:36]

On 2014/04/28 15:47:57, Alexandre Rames wrote:
> I had misunderstood your first comment. Updated the code to use both oberved
and
> required representations, but prefer the observed representation (fixes the
> `Math.log(Math.round())` test-case) and also use a double output
representation
> for tagged inputs (fixes the `Math.round(x) + "."` test-case).

I'm afraid you misunderstood my second comment too. What I suggest is (modulo
tracing):

if (use->observed_input_representation(it.index()).IsDouble() ||
    use->required_input_representation(it.index()).IsDouble()) {
  has_double_use = true;
  break;
}

Rationale:
- If you have a use that requires the value to be double-represented, it's best
to provide a double-represented value directly, rather than relying on an
HChange to do the conversion. (Assuming the math function itself is equally fast
regardless of result representation, which I understand is the case on arm64,
but likely not on other platforms. We can deal with those when we port this
optimization.)
- If you have a use that has observed double-represented values in unoptimized
code, that means there have been values earlier that couldn't be stored in an
int32 and will trigger a deopt if int32 representation is chosen and they occur
again.
- So far I don't see why preferring one data point over the other would be the
right thing to do in the general case, as I tried to demonstrate with the
examples.
- I don't like the "|| rep.IsTagged()" hack at all. It's a heuristic tuned for
the specific example you were looking at; in the general case tagged uses are
not helpful for deciding between int32 and double representations.

[Alexandre Rames, 2014-04-28 17:23:32]

I was confused. I understood the double representation part, but somehow just
messed up the code. While looking at the first example I confused 'input' and
'use' representations... I'll get some rest and update the patch tomorrow!

[Alexandre Rames, 2014-04-29 15:02:47]

Reworked the type inference.

[Jakob Kummerow, 2014-04-30 08:58:24]

Hydrogen changes LGTM.

[ulan, 2014-04-30 09:18:41]

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64.h
File src/arm64/lithium-arm64.h (right):

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64....
src/arm64/lithium-arm64.h:1912: /* Math.floor with a double result. */
Please use // comment to be consistent with other code.

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64....
src/arm64/lithium-arm64.h:1920: /* Math.floor with an integer result. */
ditto

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64....
src/arm64/lithium-arm64.h:2020: /* Math.round with an integer result. */
ditto

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64....
src/arm64/lithium-arm64.h:2031: /* Math.round with an integer result. */
ditto

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-codege...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-codege...
src/arm64/lithium-codegen-arm64.cc:4019: __ B(eq, &done);
I see how it handles 0.0 and negative integers, but how does this handle
positive input?

[Alexandre Rames, 2014-04-30 09:31:06]

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64.h
File src/arm64/lithium-arm64.h (right):

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64....
src/arm64/lithium-arm64.h:1912: /* Math.floor with a double result. */
On 2014/04/30 09:18:41, ulan wrote:
> Please use // comment to be consistent with other code.

Done.

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64....
src/arm64/lithium-arm64.h:1920: /* Math.floor with an integer result. */
On 2014/04/30 09:18:41, ulan wrote:
> ditto

Done.

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64....
src/arm64/lithium-arm64.h:2020: /* Math.round with an integer result. */
On 2014/04/30 09:18:41, ulan wrote:
> ditto

Done.

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-arm64....
src/arm64/lithium-arm64.h:2031: /* Math.round with an integer result. */
On 2014/04/30 09:18:41, ulan wrote:
> ditto

Done.

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-codege...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/258793002/diff/50001/src/arm64/lithium-codege...
src/arm64/lithium-codegen-arm64.cc:4019: __ B(eq, &done);
On 2014/04/30 09:18:41, ulan wrote:
> I see how it handles 0.0 and negative integers, but how does this handle
> positive input?

If the input is greater or equal to zero, the 'lt' (less than) condition is not
met, so FCCMP sets the Z condition flag, and the following branch is taken.

[ulan, 2014-04-30 09:47:11]

> If the input is greater or equal to zero, the 'lt' (less than) condition is
not met, so FCCMP sets the Z condition flag, and the following branch is taken.

Thanks, lgtm.

[Alexandre Rames, 2014-04-30 13:38:15]

Committed patchset #5 manually as r21091 (presubmit successful).