Closed
Bug 1076670
Opened 10 years ago
Closed 10 years ago
VS2013 Win64 jit-test failure: Math.sin(-0) returns +0
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: away, Assigned: jandem)
References
Details
Attachments
(2 files)
868 bytes,
patch
|
luke
:
review+
away
:
feedback+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
https://tbpl.mozilla.org/php/getParsedLog.php?id=49363919&tree=Date Only on VS2013 Win64 builds (we're not testing either of those on trunk yet, but will soon). The failing line of math-jit-tests.js is: testmath("Math.sin", "-0", -0) assertEq(Math.sin(-0), -0) gives me: Assertion failed: got 0, expected -0
Looks like this is the CRT's fault. The +0 comes from MSVCR120!sin. Jan, can you point me to who should decide what to do about this?
Flags: needinfo?(jdemooij)
VS2013 uses FMA3 instructions where available. My machine is pre-Haswell and took the fallback path. I'd be curious to see whether this reproduces on a Haswell+.
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to :dmajor (away 3-7 October) from comment #2) > VS2013 uses FMA3 instructions where available. My machine is pre-Haswell and > took the fallback path. I'd be curious to see whether this reproduces on a > Haswell+. I'll check.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
I downloaded the JS shell debug build [0] and Math.sin(-0) correctly returns -0. This is on Haswell (i7 4790S). It's also interesting that on TBPL [1], (Opt) jit-tests do not fail consistently. Can we easily check what CPUs the slaves have? [0] http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/date-win64-debug/1412204303/ [1] https://tbpl.mozilla.org/?rev=18e488f873e3&tree=Date
Flags: needinfo?(jdemooij)
Those green jit tests were mis-clicks. Date shows both 32-bit and 64-bit tests on the same line, and I must have re-triggered the wrong one. (You can search "build_url:" in the full log to see which.) I'm pretty sure they would fail if you re-trigger the orange one.
Assignee | ||
Comment 6•10 years ago
|
||
I created a small win32 console application and see the problem on Haswell if I build for x64 and add: _set_FMA3_enable(0);
Assignee | ||
Comment 7•10 years ago
|
||
David can you check if this fixes the problem?
Attachment #8499511 -
Flags: review?(luke)
Attachment #8499511 -
Flags: feedback?(dmajor)
Assignee | ||
Comment 8•10 years ago
|
||
I'll report this to MS later today, to make sure they are aware of this. Also note that we can probably remove this code if/when we switch to our own sin/cos implementation (bug 967709, bug 996375).
Comment 9•10 years ago
|
||
Comment on attachment 8499511 [details] [diff] [review] Patch :( Nice job looking into it.
Attachment #8499511 -
Flags: review?(luke) → review+
Assignee | ||
Comment 10•10 years ago
|
||
I filed https://connect.microsoft.com/VisualStudio/Feedback/Details/990747
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8499511 [details] [diff] [review] Patch Sorry for the delay. It seems we also get +0 for the sine of very small numbers like -2e-324. Is that something that we need to worry about?
Attachment #8499511 -
Flags: feedback?(dmajor) → feedback+
Reporter | ||
Comment 12•10 years ago
|
||
Er, nevermind, those numbers become -0 anyway so the patch addresses it.
Assignee | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/023b02665ff0
https://hg.mozilla.org/mozilla-central/rev/023b02665ff0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reporter | ||
Comment 15•10 years ago
|
||
Looks like there's a failure in another file: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3876&repo=date Failing here: http://hg.mozilla.org/mozilla-central/file/e4cfacb76830/js/src/jit-test/tests/asm.js/testMathLib.js#l13 The JIT code jumps directly to the CRT's implementation: 00000000`00110018 4883ec28 sub rsp,28h 00000000`0011001c 49bb28b1020200000000 mov r11,202B128h 00000000`00110026 493923 cmp qword ptr [r11],rsp 00000000`00110029 0f8322000000 jae 00000000`00110051 00000000`0011002f 48b8b4b4054001000000 mov rax,offset js!sin (00000001`4005b4b4) 00000000`00110039 ffd0 call rax 00000000`0011003b 6690 xchg ax,ax 00000000`0011003d 4883c428 add rsp,28h 00000000`00110041 c3 ret Was there some change in this area very recently? I thought I ran the full jit-tests on your patch. And this failure is much earlier in the test run than the previous one.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to David Major [:dmajor] (UTC+13) from comment #15) > Looks like there's a failure in another file: > https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3876&repo=date Looks like we also need to fix the asm.js code path. In js/src/asmjs/AsmJSModule.cpp, try to change this line: return RedirectCall(FuncCast<double (double)>(sin), Args_Double_Double); into: return RedirectCall(FuncCast<double (double)>(js::math_sin_uncached), Args_Double_Double);
Reporter | ||
Comment 17•10 years ago
|
||
f+, that passes jit_tests.py --tbpl.
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8503577 -
Flags: review?(luke)
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to David Major [:dmajor] (UTC+13) from comment #15) > Was there some change in this area very recently? I thought I ran the full > jit-tests on your patch. And this failure is much earlier in the test run > than the previous one. And btw, this happened because the test calls sin() from asm.js code and compares the result to the non-Odin Math.sin function. Before the first patch, both of those returned the same (incorrect) result for -0 so the test passed.
Updated•10 years ago
|
Attachment #8503577 -
Flags: review?(luke) → review+
Assignee | ||
Comment 20•10 years ago
|
||
Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/518aafb5a98a
https://hg.mozilla.org/mozilla-central/rev/518aafb5a98a
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla35 → mozilla36
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #10) > I filed https://connect.microsoft.com/VisualStudio/Feedback/Details/990747 This bug has been fixed for the next major release of Visual Studio: "Thank you for reporting this issue. This bug has been fixed for the next major release of Visual Studio, Visual Studio "14". The fix is present in the most recent preview of Visual Studio "14", which you can find here: http://www.visualstudio.com/en-us/downloads/visual-studio-14-ctp-vs.aspx."
You need to log in
before you can comment on or make changes to this bug.
Description
•