Closed
Bug 995675
Opened 10 years ago
Closed 10 years ago
Differential Testing: Different output message involving Math.cos and Math.pow
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: gkw, Assigned: h4writer)
References
Details
(Keywords: regression, testcase)
Attachments
(1 file, 1 obsolete file)
1.69 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
function f(x) { return Math.cos(~(~Math.pow(Number.MAX_VALUE, x))) } f(-0) print(f(undefined - undefined)) $ ./js-opt-64-dm-ts-linux-6149db60c6cb --fuzzing-safe --ion-parallel-compile=off test.js 1 $ ./js-opt-64-dm-ts-linux-6149db60c6cb --fuzzing-safe --ion-parallel-compile=off --ion-eager test.js NaN $ My configure flags are: AR=ar sh /home/fuzz2lin/trees/mozilla-central/js/src/configure --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options> autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/39bfcadd6492 user: Hannes Verschore date: Tue Nov 26 23:21:18 2013 +0100 summary: Bug 942105 - IonMonkey: Remove the inlineUseCountRatio option, r=jandem Hannes, is bug 942105 a possible regressor?
Flags: needinfo?(hv1989)
Assignee | ||
Comment 1•10 years ago
|
||
I am not sure why bisecting saw Bug 942105 as regressor: $ hg parent changeset: 157603:c37889c21d9f user: Timothy Nikkel <tnikkel@gmail.com> date: Tue Nov 26 16:19:41 2013 -0600 summary: Bug 943248. Enable image visibility on b2g. r=mats $ $JS/dist/bin/js --fuzzing-safe --ion-parallel-compile=off --ion-eager /tmp/test995675.js NaN $ $JS/dist/bin/js --fuzzing-safe --ion-parallel-compile=off /tmp/test995675.js 1 $ hg parent changeset: 157604:39bfcadd6492 user: Hannes Verschore <hv1989@gmail.com> date: Tue Nov 26 23:21:18 2013 +0100 summary: Bug 942105 - IonMonkey: Remove the inlineUseCountRatio option, r=jandem $ $JS/dist/bin/js --fuzzing-safe --ion-parallel-compile=off --ion-eager /tmp/test995675.js NaN $ $JS/dist/bin/js --fuzzing-safe --ion-parallel-compile=off /tmp/test995675.js 1
Flags: needinfo?(hv1989)
Assignee | ||
Comment 3•10 years ago
|
||
Bisecting gives me: The first bad revision is: changeset: 141587:c2aa14f70497 user: Brian Hackett <bhackett1024@gmail.com> date: Tue Aug 06 15:11:56 2013 -0600 summary: Bug 902009 - Fold ToDouble(ToInt32(v)) conversions, r=jandem.
Assignee | ||
Comment 4•10 years ago
|
||
GVN did something wrong to following sequence: MToInt32 MBitNot MBitNot MToDouble BitNot has an optimization that removes ~~foo to foo, which is correct for int32. It did this by testing the input to BitNot being int32. Since the input is ToInt32 that is correct. So we got: MToInt32 MToDouble ToDouble has an optimization to remove this type juggling, that it will remove ToInt32 if it is an input to ToDouble. so we got: MToDouble Which is off course incorrect. The optimization in MToDouble is only possibly if the MToInt32 is to original input to MToDouble. Here we decided to remove 2x MBitNot assuming the input is MIRType_Int32 and afterwards decided to remove the MToInt32. As solution I added in the MBitNot(MBitNot()) optimization to mark the input as having uses that have been folded. To make sure we are more carefully when looking at the input (MToInt32 in this case). And in MToDouble we check to not do this optimization if the input has uses that have been folded. Since in that case it can't decide to just remove the operation.
Assignee | ||
Comment 5•10 years ago
|
||
Note: I did test the original benchmark and the original optimization still works there (with this alteration).
Reporter | ||
Comment 6•10 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #3) > The first bad revision is: Thanks for finding the actual regressor!
Comment 7•10 years ago
|
||
Comment on attachment 8405869 [details] [diff] [review] Mark instruction as having folded uses Review of attachment 8405869 [details] [diff] [review]: ----------------------------------------------------------------- Nasty. Fix makes sense but I wonder if there are similar issues elsewhere. Please add the testcase in comment 0 (add assertEq calls for the result of the calls to f). r=me with that.
Attachment #8405869 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 8•10 years ago
|
||
After our 1:1 I decided to look closer to have the good approach. It bites us both that: ToInt32(ToDouble()) cannot always be replaced with: ToDouble() Since ToInt32 will bail on nonInt32 inputs and will possible take a different path afterwards. So I think the optimization was bogus and should be removed. Now the issue that it tried to solve was in IonBuilder::inlineMathAbs. Here we add a ToInt32() if TI only had seen Integer outputs. This used to be important, since we had to update TI as soon as we saw a double. So we added "ToInt32()" explicit to bail. Since the TI changes, we don't have to listen that strictly to TI. So we can just ignore the Int32 output type.
Attachment #8406927 -
Flags: review?(jdemooij)
Assignee | ||
Updated•10 years ago
|
Attachment #8405869 -
Attachment is obsolete: true
Comment 9•10 years ago
|
||
Comment on attachment 8406927 [details] [diff] [review] Code removal Review of attachment 8406927 [details] [diff] [review]: ----------------------------------------------------------------- Much simpler! r=me
Attachment #8406927 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e57c8c5c2ccd
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e57c8c5c2ccd
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•