IonMonkey: Specialize MMinMax for Float32

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla35
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Flags: needinfo?(benj)
Assignee

Comment 1

5 years ago
A small refactoring before the main patch.
Assignee: nobody → benj
Status: NEW → ASSIGNED
Attachment #8483600 - Flags: review?(sunfish)
Attachment #8483600 - Flags: review?(sunfish) → review+
Assignee

Comment 2

5 years ago
This implements codegen for all tier-1 platforms.
Attachment #8484238 - Flags: review?(sunfish)
Assignee

Comment 3

5 years ago
Attachment #8484240 - Flags: review?(nicolas.b.pierron)
Assignee

Comment 4

5 years ago
Attachment #8484241 - Flags: review?(sstangl)
Assignee

Comment 5

5 years ago
Attachment #8484242 - Flags: review?(luke)
Flags: needinfo?(benj)
Assignee

Comment 6

5 years ago
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 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.
Attachment #8484242 - Flags: review?(luke) → review+
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.
Attachment #8484238 - Flags: review?(sunfish) → review+
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.
Attachment #8484240 - Flags: review?(nicolas.b.pierron)
Attachment #8484241 - Flags: review?(sstangl) → review+
Assignee

Comment 10

5 years ago
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.
Attachment #8484241 - Attachment is obsolete: true
Attachment #8487917 - Flags: review?(sstangl)
Assignee

Comment 11

5 years ago
(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 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;
}
Attachment #8487917 - Flags: review?(sstangl) → review+
Assignee

Comment 13

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.