function f(x) { print(Math.pow(x, x) !== Math.pow(x, x)) } f(-0) f(-undefined) $ ./js-dbg-64-dm-nsprBuild-darwin-1b4c5daa7b7a --fuzzing-safe --no-threads --ion-eager testcase.js false false $ ./js-dbg-64-dm-nsprBuild-darwin-1b4c5daa7b7a --fuzzing-safe --no-threads --baseline-eager testcase.js false true Tested this on m-c rev 1b4c5daa7b7a. My configure flags are: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build -R ~/trees/mozilla-central" -r 1b4c5daa7b7a autoBisect is running.

autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/e5d631abcd56 user: Tom Schuster date: Sun Oct 05 15:26:40 2014 +0200 summary: Bug 1073576 - Optimize strict compares with equal operands. r=h4writer Not sure if bug 1073576 being the regressor is entirely accurate. Hannes, thoughts?

It is debatable if bug 1073576 is the real cause. But by doing that we opened a floodgate of bugs. This is the same bug as bug 1130679. Actively looking into that. (Btw thanks for all the different testcases. Giving me new ideas/ways to run into this).

After careful examination of the testcase, this is indeed partially bug 1130679. But it also uncovers an additional bug. MToInt32 (and others) are not recognized as filtering types, which it definitely does! As a result the guards put in place to not remove this instruction are gone and we get this wrong result. The issue is because we are working with two type of ranges: 1) Full range an instruction can output giving the range of the inputs (i.e. before bailouts) 2) Range of an instructions that uses only will see (i.e. after bailouts) And the distinction between those two ranges are not always correct and done correctly. e.g. MToInt32 - range after computation before bailouts: input()->range() - range after bailouts: intersection of input()->range() and MIRType_Int32 Currently MToInt32 gives the later range for both. So we don't recognize this instruction as filtering types!

So the issue here is: MToInt32::computeRange which contains the range *after* bailouts. - range() should contain the range *before* bailouts! - (also some functions use "MDefinition::range()" instead of more specific Range(ins)) Honest mistakes. So I propose: -> adding "MDefinition::getOutputRange()" which should get used almost everywhere. Which contains the range *after* bailouts. -> maybe changing "MDefinition::range()" to MDefinition::innerRange() ? s/computeRange/computeInnerRange/g -> make it possible to override "MDefinition::getOutputRange()", so MToInt32 can adjust the range *after* bailouts. This would allow other functions to override their *after* range. (Which e.g. MUrsh already wants to do). This patch already fixes this bug. What do you think?

(In reply to Hannes Verschore [:h4writer] from comment #4) > So I propose: > -> adding "MDefinition::getOutputRange()" which should get used almost > everywhere. Which contains the range *after* bailouts. > -> maybe changing "MDefinition::range()" to MDefinition::innerRange() ? > s/computeRange/computeInnerRange/g > -> make it possible to override "MDefinition::getOutputRange()", so MToInt32 > can adjust the range *after* bailouts. > > This patch already fixes this bug. > What do you think? As Range Analysis is a complex beast which involve a lot of uplifts, (as you might discover), I would prefer if you can fix this the bad way first (by adding things in the Range constructor), and then make focus clean-up patches which are not changing the semantics. To be honest, I like the look of the Range constructor syntax, as it is easy to distinguish from the pointer syntax, which simplifies the reviews a lot. I think this would be fine if the Range constructor was initialized based on the returned value of the MDefinition::returnRange() function. Also inner is to opposed to outer, and output is opposed to input. I think just having range and returnedRange would be enough. I do not see the need for the renaming of the computeRange function.

Sub-optimal: found two instructions that are still using the computed range instead of the output range.

Comment on attachment 8570410 [details] [diff] [review] Optim: Use output range Review of attachment 8570410 [details] [diff] [review]: ----------------------------------------------------------------- Good catch!

Comment on attachment 8570410 [details] [diff] [review] Optim: Use output range Review of attachment 8570410 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 8570408 [details] [diff] [review] Minimal patch Review of attachment 8570408 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/RangeAnalysis.cpp @@ +1641,1 @@ > setRange(output); Please leave a comment here mentioning why we're not doing a clamp here. I guess something along the lines of: "MToInt32 enforces its int32 type in bailouts, so the pre-bailout range can be considered unclamped".

https://hg.mozilla.org/integration/mozilla-inbound/rev/15fc680d0c8b https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8219a083f8

https://hg.mozilla.org/mozilla-central/rev/15fc680d0c8b https://hg.mozilla.org/mozilla-central/rev/7e8219a083f8