function g(f, x) { for (var j = 0; j < 2; ++j) { for (var k = 0; k < 3; ++k) { print(f(x[j], x[1])) } } } with({}) {} (function() { function f(x) { return (-0x80000000 == -(x | 0)) } g(f, [0xf, 0x80000000]) })() $ ./js-opt-64-dm-ts-darwin-582b2d81ebe1 --fuzzing-safe --ion-parallel-compile=off 300.js false false false false false false $ ./js-opt-64-dm-ts-darwin-582b2d81ebe1 --fuzzing-safe --ion-parallel-compile=off --ion-eager 300.js false false false true true true (Tested this on 64-bit Mac js opt threadsafe deterministic shell off m-c rev 582b2d81ebe1) My configure flags (Mac) are: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --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/9658dbcf4cd7 user: Dan Gohman date: Mon Dec 09 07:11:12 2013 -0800 summary: Bug 943303 - IonMonkey: Convert floating-point comparisons to integer using range analysis. r=nbp Dan, is bug 943303 a likely regressor?

It's likely.

This patch generalizes range analysis truncation to also support a new form of conversion to integer which still requires full bailout checks. This allows us to more precisely describe what happens when we convert an MCompare to an integer. It doesn't silently truncate its operands, but it still wants them to have integer types if possible. While here, this patch also incorporates indirect truncation as well, eliminating the code that did an extra use-list traversal.

Here's another testcase that is fixed by the patch here: function f(x) { return 0 > -0 + -~x } f(0) print(f(2147483647)) $ ./js-opt-64-dm-ts-darwin-3b166b8add93 --fuzzing-safe --ion-parallel-compile=off 10508.js false $ ./js-opt-64-dm-ts-darwin-3b166b8add93 --fuzzing-safe --ion-parallel-compile=off --ion-eager 10508.js true

Comment on attachment 8411443 [details] [diff] [review] fix-kinds.patch Review of attachment 8411443 [details] [diff] [review]: ----------------------------------------------------------------- I am not sure to understand why we need this renaming of fix -> truncate? "Fix" sounds like this it was originally an error and that we have to do it. "Truncate" is more descriptive and sounds optional. Note, this patch entirely disable the truncation of comparison operands, as comparisons are lowering the "fix" (truncation level) to a level which is never used for truncating anything. ::: js/src/jit/MIR.h @@ +421,5 @@ > + FixWithBailouts, > + // The value will be truncated after some arithmetic (see above). > + IndirectTruncate, > + // Direct and infallible truncation to int32. > + Truncate nit: As you are using the Min/Max functions on the FixKind, I'll suggest that we provide a numbering, even if it is implicit, and comment that we use it to merge truncation kinds. @@ +3532,5 @@ > return AliasSet::None(); > } > > bool isTruncated() const { > + return implicitFix_ == Truncate; Sadly, even if this code is correct for MDiv, it also de-optimize other arithmetic operations. A simple test case such as: function f(x) { var a = x | 0; return (a + a + a + a) | 0; } Should not use doubles. MAdd::fallible() function should check that the operation is at least implicitly truncated, instead of checking isTruncated. ::: js/src/jit/RangeAnalysis.cpp @@ +2167,5 @@ > > if (type() == MIRType_Double || type() == MIRType_Int32) { > specialization_ = MIRType_Int32; > setResultType(MIRType_Int32); > + if (kind == Truncate && range()) nit: kind >= IndirectTruncate @@ +2184,5 @@ > > if (type() == MIRType_Double || type() == MIRType_Int32) { > specialization_ = MIRType_Int32; > setResultType(MIRType_Int32); > + if (kind == Truncate && range()) nit: kind >= IndirectTruncate @@ +2201,5 @@ > > if (type() == MIRType_Double || type() == MIRType_Int32) { > specialization_ = MIRType_Int32; > setResultType(MIRType_Int32); > + if (kind == Truncate) { nit: kind >= IndirectTruncate @@ +2265,4 @@ > // We use the return type to flag that this MToDouble should be replaced by > // a MTruncateToInt32 when modifying the graph. > setResultType(MIRType_Int32); > + if (kind == Truncate) { nit: kind >= IndirectTruncate

(In reply to Nicolas B. Pierron [:nbp] from comment #5) > Comment on attachment 8411443 [details] [diff] [review] > fix-kinds.patch > > Review of attachment 8411443 [details] [diff] [review]: > ----------------------------------------------------------------- > > I am not sure to understand why we need this renaming of fix -> truncate? > "Fix" sounds like this it was originally an error and that we have to do it. > "Truncate" is more descriptive and sounds optional. I was trying to avoid ambiguity with "truncate" meaning either "full truncate" or something other kind of truncate. I reverted this now. > Note, this patch entirely disable the truncation of comparison operands, as > comparisons are lowering the "fix" (truncation level) to a level which is > never used for truncating anything. This is as intended. Comparison isn't a full-truncating operation. Before this patch, truncation mainly because it was the only way to get operands into integer registers. This patch introduces a way to get the operands into integer registers without actually truncating them. > ::: js/src/jit/MIR.h > @@ +421,5 @@ > > + FixWithBailouts, > > + // The value will be truncated after some arithmetic (see above). > > + IndirectTruncate, > > + // Direct and infallible truncation to int32. > > + Truncate > > nit: As you are using the Min/Max functions on the FixKind, I'll suggest > that we provide a numbering, even if it is implicit, and comment that we use > it to merge truncation kinds. Ok. > @@ +3532,5 @@ > > return AliasSet::None(); > > } > > > > bool isTruncated() const { > > + return implicitFix_ == Truncate; > > Sadly, even if this code is correct for MDiv, it also de-optimize other > arithmetic operations. > A simple test case such as: > > function f(x) { > var a = x | 0; > return (a + a + a + a) | 0; > } > > Should not use doubles. > > MAdd::fallible() function should check that the operation is at least > implicitly truncated, instead of checking isTruncated. Thanks for spotting that. I fixed this, and your simple testcase is once again codegen'd without overflow checks. > ::: js/src/jit/RangeAnalysis.cpp > @@ +2167,5 @@ > > > > if (type() == MIRType_Double || type() == MIRType_Int32) { > > specialization_ = MIRType_Int32; > > setResultType(MIRType_Int32); > > + if (kind == Truncate && range()) > > nit: kind >= IndirectTruncate That's pretty subtle, but I think it's right.

Comment on attachment 8415665 [details] [diff] [review] truncate-kinds.patch Review of attachment 8415665 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +421,5 @@ > + // 3. Ignore negative zeros. (-0 --> 0) > + // 4. Ignore remainder. (3 / 4 --> 0) > + // > + // Indirect truncation is used to represent that we are interested in the > + // truncated result, but only if they it can safely flow in operations which self-nit: … if >they< it can … @@ +2972,5 @@ > #endif > + > + TruncateKind truncateKind() const { > + return implicitTruncate_; > + } style-nit: fix trailing white-spaces.

(In reply to Nicolas B. Pierron [:nbp] from comment #7) > self-nit: … if >they< it can … > style-nit: fix trailing white-spaces. Fixed and fixed. https://hg.mozilla.org/integration/mozilla-inbound/rev/695049af7654

The patch had the wrong bug number. The following patches revert it and reapply it with the correct number: http://hg.mozilla.org/integration/mozilla-inbound/rev/87feb441d562 http://hg.mozilla.org/integration/mozilla-inbound/rev/75413bfa9dd8

https://hg.mozilla.org/integration/mozilla-inbound/rev/23d16c2f0f67 Backed out for causing: Bug 1006301 Bug 1006561 Bug 1006850 (Ref bug 1006301 comment 5)

Backout made it to m-c: https://hg.mozilla.org/mozilla-central/rev/23d16c2f0f67

(In reply to Carsten Book [:Tomcat] from comment #13) > https://hg.mozilla.org/mozilla-central/rev/b66e279688a1 This landed as part of bug 1006301.