Closed Bug 941381 Opened 11 years ago Closed 11 years ago

IonMonkey: Incorrect fround(asin())

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox26 --- unaffected
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: jruderman, Assigned: bbouvier)

References

Details

(Keywords: regression, testcase, Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

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);
Attached file 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
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.
> Created attachment 8335736 [details]
> bisect log

I pushed fuzzing rev 8053b2cd842f to ignore these changesets.
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?
Attached patch Best typo ever (obsolete) — Splinter Review
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).
Assignee: nobody → benj
Status: NEW → ASSIGNED
Attachment #8335985 - Flags: review?(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 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.
Attachment #8335985 - Flags: review?(luke) → review+
Per the release calendar, setting status flags.
https://hg.mozilla.org/mozilla-central/rev/4d895f46b99b
https://hg.mozilla.org/mozilla-central/rev/2dada415ebba
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite+
Whiteboard: [qa-]
Attached patch Uplift patchSplinter Review
[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.
Attachment #8335985 - Attachment is obsolete: true
Attachment #8371371 - Flags: review+
Attachment #8371371 - Flags: approval-mozilla-release?
FWIW, this has been noticed by users, in bug 968522.
Blocks: 968522
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.
Attachment #8371371 - Flags: approval-mozilla-release? → approval-mozilla-release+
I was hoping you'd land the tests for this too. I was going to push this with tests this morning.
Flags: needinfo?(benj)
^ 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?
(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.
You need to log in before you can comment on or make changes to this bug.