1053788 - IonMonkey: Specialize MMinMax for Float32

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

There are minss and maxss instructions for x86, and I don't see any reason why MMinMax wouldn't be float32-commutative (comparisons are float32 commutative), so we could specialize it.

Will do unless somebody beats me to it.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1053788-refactor.patch

A small refactoring before the main patch.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2: Backends for x86, x64, ARM

This implements codegen for all tier-1 platforms.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 3: Update the recover instruction RMinMax

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 4: Activate float32 specialization of MMinMax

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 5: Add float32 specialization of Math.min/max in Odin

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Modified test files all pass with --tbpl. --ion test pass with a x64 build. Try run for sanity:
https://tbpl.mozilla.org/?tree=Try&rev=75cc74cbb606

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8484242 [details] [diff] [review]
Part 5: Add float32 specialization of Math.min/max in Odin

Review of attachment 8484242 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/AsmJSValidate.cpp
@@ +4305,1 @@
>      bool opIsInteger = firstType.isInt();

I was thinking, perhaps we should finally just add a:
  bool operator<=(Type lhs, Type rhs);
overload so that we can remove these 3 bools and have a single T argType and check below with:
  if (!(nextType <= argType))
We could reuse this to avoid this same situation in CheckMathBuiltinCall.  Up to you.

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8484238 [details] [diff] [review]
Part 2: Backends for x86, x64, ARM

Review of attachment 8484238 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: js/src/jit/Lowering.cpp
@@ +1278,5 @@
>  
> +    if (ins->specialization() == MIRType_Float32) {
> +        LMinMaxF *lir = new(alloc()) LMinMaxF(useRegisterAtStart(first), useRegister(second));
> +        return defineReuseInput(lir, ins, 0);
> +    }

This is a comment on the original code, but there should probably be an assert that ins->specialization() == MIRType_Double here.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8484240 [details] [diff] [review]
Part 3: Update the recover instruction RMinMax

Review of attachment 8484240 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/Recover.cpp
@@ +761,5 @@
>  
> +    // MIRType_Float32 is a specialization embedding the fact that the result is
> +    // rounded to a Float32.
> +    if (isFloatOperation_ && !RoundFloat32(cx, result, &result))
> +        return false;

As the operands are already float32, does it make sense to have a roundFloat32 here?
I am fine for adding the tests cases (r=me), but I do not see the point into adding the code.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 4: Activate float32 specialization of MMinMax

Sorry Sean, I had a little epiphany: we don't need to look at consumers of Math.min/max when deciding whether to specialize or not. As Math.min/max is semantically equivalent to a comparison and an assignment, the output doesn't need to be validated.

Even better: Math.min/max with more than 2 arguments creates several MMinMax, so we can specialize the trySpecializeFloat32 function so that it looks at inputs which also are MMinMax that have been specialized for float32. So e.g. Math.min(f32[0], f32[1], f32[2]) can entirely be carried on with single precision instructions.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> As the operands are already float32, does it make sense to have a
> roundFloat32 here?
You're right. I've modified the patch to just add the test cases and r=you.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8487917 [details] [diff] [review]
Part 4: Activate float32 specialization of MMinMax

Review of attachment 8487917 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/ion/testFloat32-correctness.js
@@ +103,5 @@
> +    f32[9] = Math.pow(2,31) - 1;
> +}
> +function minMax() {
> +    for(var i = 0; i < 9; ++i) {
> +        var minf = Math.fround(Math.min(f32[i], f32[i+1]));

nit:

for(var i = 0; i < 9; ++i) {
  for(var j = 0; j < 9; ++j) {
    var minf = Math.fround(Math.min(f32[i], f32[j]));

::: js/src/jit-test/tests/ion/testFloat32.js
@@ +210,5 @@
> +    var res = Math.max(f32[0], 42.13 + f32[1]);
> +    assertFloat32(res, false);
> +    f64[0] = res;
> +}
> +test(refuseMax);

Add additional test case, where you check that the output of MinMax can fit as any other Math.fround(…) result:

function acceptMinAdd() {
    var res = Math.min(f32[0], f32[1]) + f32[2];
    assertFloat32(res, true);
    f32[3] = res;
}

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> ::: js/src/jit-test/tests/ion/testFloat32.js
> @@ +210,5 @@
> > +    var res = Math.max(f32[0], 42.13 + f32[1]);
> > +    assertFloat32(res, false);
> > +    f64[0] = res;
> > +}
> > +test(refuseMax);
> 
> Add additional test case, where you check that the output of MinMax can fit
> as any other Math.fround(…) result:
> 
> function acceptMinAdd() {
>     var res = Math.min(f32[0], f32[1]) + f32[2];
>     assertFloat32(res, true);
>     f32[3] = res;
> }

This last test wouldn't work, as MMinMax isn't a float32 producer. However, MMinMax behaves like a Phi (which makes sense, as it's a comparison and an assignment): it is a producer if the inputs are producers, and it is a consumer if all uses are consumers. While the concept is pretty simple, the implementation would involve caching these two values, as otherwise chains of MMinMax would involve a lot of recursive calls (MMinMax::canProduceFloat32 would just consist in calling canProduceFloat32 on its two inputs). nbp and I agreed AFK this wasn't worth it, at least in the scope of this bug.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac587fb9cda
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5f142d472a26
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bad63e9d0b19
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4dda6de2f09a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b54c2c8785

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ac587fb9cda
https://hg.mozilla.org/mozilla-central/rev/5f142d472a26
https://hg.mozilla.org/mozilla-central/rev/bad63e9d0b19
https://hg.mozilla.org/mozilla-central/rev/4dda6de2f09a
https://hg.mozilla.org/mozilla-central/rev/b1b54c2c8785