Closed
Bug 686323
Opened 13 years ago
Closed 13 years ago
ion: div bug
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: evilpies, Assigned: evilpies)
Details
Attachments
(3 files, 1 obsolete file)
451 bytes,
application/javascript
|
Details | |
26.57 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
845 bytes,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Ion seems to have some bug regarding, division by zero behavior.
This should fix the constant folding (also it seems that this doesn't work?!).
Also i found one small optimization that we used to have in Jäger when checking for 0 and INT_MIN.
Looking at the spew it looks like the b/0 is optimized away?
Also i am not convinced that the inline code for div is valid (How did Jäger handle that?)
Comment on attachment 559869 [details] [diff] [review]
small fix for two issues
Review of attachment 559869 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/MIR.cpp
@@ +127,5 @@
> + else
> + ret.setNumber(js_PositiveInfinity);
> + } else {
> + ret.setNumber(d1 / d2);
> + }
I see this is in JM too - could we factor it out into jsnum, like js_fmod?
::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ -476,1 +467,1 @@
> > Label nonzero;
This removes the -2147483648 / -1 check:
$ ./Debug32/js --ion --ion-eager jit-test/tests/ion/testDivide.js
Floating point exception
>I see this is in JM too - could we factor it out into jsnum, like js_fmod?
Sure.
>This removes the -2147483648 / -1 check:
fail! i somehow read masm.cmpl(>rhs<, Imm32(INT_MIN)); all the freaking time.
Any idea what causes the other issues?
(Note: jsinterp.cpp has the same logic too, so we could remove it from three places)
Are there issues other than the constant folding? testD() seems to pass for me.
>tom@tom-linux:~/ionmonkey/js/src/build-debug/shell$ ./js --ion test.js
>test.js:28: Error: Assertion failed: got 1, expected -Infinity
Attachment #559949 -
Flags: review?(dvander)
Attachment #559869 -
Attachment is obsolete: true
![]() |
||
Updated•13 years ago
|
Attachment #559949 -
Flags: review?(dvander) → review+
![]() |
||
Updated•13 years ago
|
Attachment #559950 -
Flags: review+
Do not resolve please, this bug isn't fixed yet.
The unrelated base patch is http://hg.mozilla.org/integration/mozilla-inbound/rev/c614dabf94e2.
Comment 9•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c614dabf94e2
(I'll nom this bug for the most obscure summary ever! :P)
Target Milestone: --- → mozilla9
Comment 10•13 years ago
|
||
This code doesn't make any sense to me:
if (a == 0 || JSDOUBLE_IS_NaN(a)
#ifdef XP_WIN
|| JSDOUBLE_IS_NaN(a) /* XXX MSVC miscompiles such that (NaN == 0) */
#endif
I think the second JSDOUBLE_IS_NAN(a) should be JSDOUBLE_IS_NAN(b).
Also, it would surprise me if MSVC > 6 still botches NaNs like this.
Comment 11•13 years ago
|
||
Comment 10 is correct. This needs fixing on inbound before m-c promotes to Aurora.
@dvander: OK to push the above change?
pete, good catch, thanks!
sstangl, yeah please, r=me on that fix
Comment 13•13 years ago
|
||
Comment 14•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•