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)

x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: gkw, Assigned: h4writer)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file, 1 obsolete file)

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)
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)
I need to recheck this, sorry about that.
Flags: needinfo?(gary)
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.
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: nobody → hv1989
Attachment #8405869 - Flags: review?(jdemooij)
Flags: needinfo?(gary)
Note: I did test the original benchmark and the original optimization still works there (with this alteration).
(In reply to Hannes Verschore [:h4writer] from comment #3)
> The first bad revision is:

Thanks for finding the actual regressor!
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+
Attached patch Code removalSplinter Review
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)
Attachment #8405869 - Attachment is obsolete: true
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+
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.