941381 - IonMonkey: Incorrect fround(asin())

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

With --ion-eager, fails with what might be a rounding difference:

function f() { return Math.fround(Math.asin(0.015625)) }
assertEq(f(), f());

With --ion-eager, fails with a much larger difference:

function g() { return Math.fround(Math.asin(2)) }
assertEq(isNaN(g()), true);
assertEq(isNaN(g()), true);

Comment 1

[Jesse Ruderman]

Attachment: bisect log

Of these 8 changesets, I suspect the regressor was:

changeset:   http://hg.mozilla.org/mozilla-central/rev/e1226725f674
user:        Benjamin Bouvier
date:        Thu Oct 17 08:50:56 2013 +0200
summary:     [Bug 918163] - Specialize some Maths function calls for Float32 in Ion. r=sstangl

Comment 2

[Luke Wagner [:luke]]

Unlike arithmetic, sin/cos/etc do not have a exactly-defined result and there is not a commutative identity as there is with +,-,*,/.  Asking the Intel FP people about this, they said that there is a standard unit of variation accepted for trig functions (in any C standard library) and that, for fround'ed values, sin(float) vs. sin(double) would be within this unit of variance.  So, depending on what the delta is, that would explain the first example.

The second example is more concerning.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Created attachment 8335736 [details]
> bisect log

I pushed fuzzing rev 8053b2cd842f to ignore these changesets.

Comment 4

[Jesse Ruderman]

Should we disable the f32 optimization for trig functions, or should we accept that fround(trig*()) is unstable and try to disable differential fuzzing for that combination?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Best typo ever

Ahem. Typos! Very nice find.

That made me realize that the Float32 tests that check correctness for the trigo functions were not asserting. I created a new test file that I will land with this patch, once I'll be sure that it doesn't break any platform (try).

Comment 6

[Luke Wagner [:luke]]

Haha, turns out it was a real bug :)  As for the observability of sin vs. sinf: this should be within the spec and within expectations, so I think we're fine.  What I don't know is how easy it is to observe; we can wait and see if the fuzzers find anything but, if they do it should be a small delta so maybe we could use an assertWithinEpsilon() function instead of assertEq().

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8335985 [details] [diff] [review]
Best typo ever

Haha.  Can you remove the extra space before 'break' so they all line up?  r+ with test.

Comment 8

[Florian Bender]

Per the release calendar, setting status flags.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Tests all green on try!

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d895f46b99b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dada415ebba

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d895f46b99b
https://hg.mozilla.org/mozilla-central/rev/2dada415ebba

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Uplift patch

[Approval Request Comment]
Regression caused by (bug #): 918163
User impact if declined: important correctness issues
Testing completed (on m-c, etc.): m-i and m-c for a while
Risk to taking this patch (and alternatives if risky): no risk
String or IDL/UUID changes made by this patch: n/a

Got R+ from luke.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

FWIW, this has been noticed by users, in bug 968522.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8371371 [details] [diff] [review]
Uplift patch

We can take this as a ride-along with the stability-driven 27.0.1 release that will go out at the end of this week.  Please land this to the mozilla-release branch asap in order for it to get into tomorrow's builds.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/releases/mozilla-release/rev/feece5efc4eb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I was hoping you'd land the tests for this too. I was going to push this with tests this morning.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Sure thing!

https://hg.mozilla.org/releases/mozilla-release/rev/a5a4322d43f3

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

^ Annnnnd this is how I break mozilla-release... :(

And this is how I fix it:
https://hg.mozilla.org/releases/mozilla-release/rev/66b0d478ee77

FTR, it's a part of one of the patches in bug 930477 (landed one month before this landing, that's why we didn't catch it here), where the same issue showed up.

Should I ask for an a-posteriori approval for this one?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> And this is how I fix it:
> https://hg.mozilla.org/releases/mozilla-release/rev/66b0d478ee77

Looks like it worked, thanks :)

> Should I ask for an a-posteriori approval for this one?

Nah, I think a=bustage and passing tests is good enough.