[arm64] Implement Float(32|64)(Min|Max) using fcsel.

[arm64] Implement Float(32|64)(Min|Max) using fcsel.

Float(32|64)Min:
  // (a < b) ? a : b
  fcmp da, db
  fcsel dd, da, db, lo

Float(32|64)Max:
  // (b < a) ? a : b
  fcmp db, da
  fcsel dd, da, db, lo

BUG=

Committed: https://crrev.com/2f80165f226c64010c40ba597710039fa8f2c8b6
Cr-Commit-Position: refs/heads/master@{#31621}

[jbramley, 2015-09-21 15:58:53]

jacob.bramley@arm.com changed reviewers:
+ bmeurer@chromium.org, ulan@chromium.org

[Benedikt Meurer, 2015-09-21 17:31:43]

Hey Jacob,

Nice patch. I'm not 100% about the approach, tho. One independent question: What
is the impact of using fcsel instead of the diamond?

Benedikt

https://codereview.chromium.org/1360603003/diff/1/src/compiler/arm64/code-gen...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1360603003/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:750: case
kArm64Float32CmpAndFloat32Sel:
Hm, this instruction looks redundant. Why do you need it?

https://codereview.chromium.org/1360603003/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:793: case
kArm64Float64CmpAndFloat64Sel:
Same here?

https://codereview.chromium.org/1360603003/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:1085: case
kArm64Float32CmpAndFloat32Sel: {
I'm not sure if this approach scales well. Because the select is actually
independent of the comparison, the operands of the comparison, conceptionally.
Did you consider using different kinds of Select, i.e. SelectFloat32,
SelectFloat64 and so on?

[jbramley, 2015-09-22 07:38:39]

Benedikt,

> Nice patch. I'm not 100% about the approach, tho. One independent question:
What
> is the impact of using fcsel instead of the diamond?

In general, I don't know, and that's why I've only used fcsel for min/max.
Instinctively, fcsel might be better for unpredictable outcomes, but a diamond
might be better for predictable cases, but this depends on microarchitecture.
Using fcsel for min/max seems to give about 1% for bullet and box2d. (Those are
the only two benchmarks I've seen that use TF's min/max.)

This is a kind of best-effort replacement for the real fmin/fmax instructions,
which unfortunately we can't use in TF:
https://code.google.com/p/v8/issues/detail?id=4206.

Thanks,
Jacob

[jbramley, 2015-09-22 07:45:21]

https://codereview.chromium.org/1360603003/diff/1/src/compiler/arm64/code-gen...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/1360603003/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:750: case
kArm64Float32CmpAndFloat32Sel:
On 2015/09/21 17:31:43, Benedikt Meurer wrote:
> Hm, this instruction looks redundant. Why do you need it?

Because, although I haven't done so yet, we might want the type of the select
arguments to differ from the type of the comparison. Encoding that in the
architecture-specific part of the opcode seemed the most sensible thing to do.

https://codereview.chromium.org/1360603003/diff/1/src/compiler/arm64/code-gen...
src/compiler/arm64/code-generator-arm64.cc:1085: case
kArm64Float32CmpAndFloat32Sel: {
On 2015/09/21 17:31:43, Benedikt Meurer wrote:
> I'm not sure if this approach scales well. Because the select is actually
> independent of the comparison, the operands of the comparison, conceptionally.
> Did you consider using different kinds of Select, i.e. SelectFloat32,
> SelectFloat64 and so on?

Indeed, but (unless I'm mistaken), TF can't track flags dependencies between
instructions. In all other cases, instructions that produce flags always consume
them (either explicitly or using a FlagsContinuation).

Is there a better way to handle that?

[Benedikt Meurer, 2015-09-22 08:40:30]

bmeurer@chromium.org changed reviewers:
+ jarin@chromium.org, titzer@chromium.org

[Benedikt Meurer, 2015-09-22 08:40:31]

+titzer, +jarin for more opinions from the other compiler folks.

You're right, this is probably the easiest way to implement that without
changing too much of the backend. And that also works well for the min/max use
cases. I dunno if we ever want to support something other than mix/max like
things with selects.

[jbramley, 2015-10-05 14:54:26]

On 2015/09/22 08:40:31, Benedikt Meurer wrote:
> +titzer, +jarin for more opinions from the other compiler folks.

Any other comments?

[Benedikt Meurer, 2015-10-26 09:24:22]

As discussed offline with Rodolph and Baptiste, we'd prefer to have a more local
solution for now. So basically for this CL, special case the
Float(32|64)(Min|Max) backend opcodes and do the comparison inside the opcode
instead of passing the conditions using Select flags magic.

[jbramley, 2015-10-27 14:15:10]

Description was changed from

==========
[arm64] Implement Float(32|64)(Min|Max) using fcsel.

Float(32|64)Min:
  // (a < b) ? a : b
  fcmp da, db
  fcsel dd, da, db, lo

Float(32|64)Max:
  // (b < a) ? a : b
  fcmp db, da
  fcsel dd, da, db, lo

Conditional select isn't currently used for anything other than min/max.

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

to

==========
[arm64] Implement Float(32|64)(Min|Max) using fcsel.

Float(32|64)Min:
  // (a < b) ? a : b
  fcmp da, db
  fcsel dd, da, db, lo

Float(32|64)Max:
  // (b < a) ? a : b
  fcmp db, da
  fcsel dd, da, db, lo

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

[jbramley, 2015-10-27 14:49:04]

On 2015/10/26 09:24:22, Benedikt Meurer wrote:
> As discussed offline with Rodolph and Baptiste, we'd prefer to have a more
local
> solution for now. So basically for this CL, special case the
> Float(32|64)(Min|Max) backend opcodes and do the comparison inside the opcode
> instead of passing the conditions using Select flags magic.

A new patch is ready. (I'm making a comment because the patch alone doesn't seem
to have triggered an e-mail to the list.)

[Benedikt Meurer, 2015-10-27 19:02:59]

LGTM with nit.

https://codereview.chromium.org/1360603003/diff/20001/src/compiler/arm64/inst...
File src/compiler/arm64/instruction-codes-arm64.h (right):

https://codereview.chromium.org/1360603003/diff/20001/src/compiler/arm64/inst...
src/compiler/arm64/instruction-codes-arm64.h:86: V(Arm64Float32TernaryMax)      
 \
Nit: I'd prefer to keep the FloatXX(Min|Max) names here. That way it is
consistent with the other backends. The Math.min/.max operator can then be
consistently named Float64NanSensitiveMax or something like that.

[jbramley, 2015-10-28 09:34:52]

The CQ bit was checked by jacob.bramley@arm.com

[jbramley, 2015-10-28 09:34:53]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1360603003/#ps40001
(title: "Remove "Ternary".")

[commit-bot: I haz the power, 2015-10-28 09:35:02]

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

[jbramley, 2015-10-28 09:42:10]

https://codereview.chromium.org/1360603003/diff/20001/src/compiler/arm64/inst...
File src/compiler/arm64/instruction-codes-arm64.h (right):

https://codereview.chromium.org/1360603003/diff/20001/src/compiler/arm64/inst...
src/compiler/arm64/instruction-codes-arm64.h:86: V(Arm64Float32TernaryMax)      
 \
On 2015/10/27 19:02:59, Benedikt Meurer wrote:
> Nit: I'd prefer to keep the FloatXX(Min|Max) names here. That way it is
> consistent with the other backends. The Math.min/.max operator can then be
> consistently named Float64NanSensitiveMax or something like that.

Acknowledged.

[commit-bot: I haz the power, 2015-10-28 09:54:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-28 09:55:20]

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