function f(a, b) { return Math.ceil((a | 0) / (b | 0)) | 0 } x = 0x80000000 f(x, x) print(f(-0xf, x)) $ ./js-opt-64-dm-ts-darwin-33ca5e321046 --fuzzing-safe --ion-parallel-compile=off 2021.js 1 $ ./js-opt-64-dm-ts-darwin-33ca5e321046 --fuzzing-safe --ion-parallel-compile=off --ion-eager 2021.js 0 (Tested this on 64-bit Mac js opt threadsafe deterministic shell off m-c rev 33ca5e321046, and I think it also reproduces on Linux) 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/e3e21c3ada8c user: Benjamin Bouvier date: Thu Dec 19 15:32:59 2013 +0100 summary: Bug 936740: inline call to libc's ceil for Math.ceil(); r=jandem Benjamin, is bug 936740 likely related?

I tried to reduce the test case, but couldn't do more than that: function f(a, b) { var div = a / b; //print(div) var ceil = Math.ceil(div) //print(ceil) return ceil | 0 } x = 0x80000000 | 0 f(x, x) print(f(-15, x)) Funny thing: if any of the print statements is uncommented, the division is observable and the result is correct with --ion-eager. Looking at the generated graph with iongraph, it appears that the division is truncated to an int32 (!), then the ceil operation is just eliminated (as ceil(X) == X for all X in the Int32 set). I remember nbp and / or sunfish modified integer division truncation recently, so bouncing the needinfo.

The problem is indeed related to Bug 936740, as the optimization made for Ceil[1] is based on wrong assumptions. MDiv might have a MIRType_Int32 if we only observed integers as result. In which case it is fine to remove the MMathFunction for Ceil, but it is not fine to replace it by nothing. We should make add a No-op MIR Node which does not transfer the truncation, but only an indirect truncation. The problem is that the Division bailouts are removed because we consider it as being directly truncated. [1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MCallOptimize.cpp#682

I added a Nop1, which is used to prevent accepting greedy truncations. I also made a test case is testing enough cases to highlight the differences between the 4 mode of truncations. I will open another bug to only guard on the remainder and not on infinities of MDiv, as well as adding computeRange functions for these Math operations.

Comment on attachment 8418084 [details] [diff] [review] Prevent division's truncation after removal of Math functions. Review of attachment 8418084 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MCallOptimize.cpp @@ +653,5 @@ > callInfo.setImplicitlyUsedUnchecked(); > + MNop1 *ins = MNop1::NewRestrictTruncate(alloc(), callInfo.getArg(0), > + MDefinition::IndirectTruncate); > + current->add(ins); > + current->push(ins); A brief comment here would be useful. How about "The int operand may be something which bails out if the actual value isn't in range. We need to tell the optimizer to preserve this bailout even if the final result is fully truncated." ::: js/src/jit/MIR.h @@ +967,5 @@ > } > }; > > +// No-op instruction. This cannot be moved or eliminated, and is intended for > +// protecting the input against follow-up optimization. Unlike the existing MNop, this class can be moved (e.g. LICM) and eliminated (e.g. DCE). Also, the embedded truncation logic makes this more specialized than its current name suggests. How about naming this class "MLimitedTruncate"?

Comment on attachment 8418084 [details] [diff] [review] Prevent division's truncation after removal of Math functions. Review of attachment 8418084 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, only a few optional style nits. r+ with dan's comments applied. ::: js/src/jit/MIR.h @@ +967,5 @@ > } > }; > > +// No-op instruction. This cannot be moved or eliminated, and is intended for > +// protecting the input against follow-up optimization. +1 on the renaming. It would be nice to have something on the name that says unary. @@ +990,5 @@ > + static MNop1 *NewRestrictTruncate(TempAllocator &alloc, MDefinition *input, TruncateKind kind) { > + return new(alloc) MNop1(input, kind); > + } > + > + MDefinition *input() { nit: this method is already present in MUnaryInstruction, modulo constness. There's a risk of hidding the original method. Doesn't gcc complain about it? ::: js/src/jit/RangeAnalysis.cpp @@ +2289,5 @@ > +MNop1::truncate(TruncateKind kind) > +{ > + setTruncateKind(kind); > + setResultType(MIRType_Int32); > + if (kind >= IndirectTruncate) { style nit: if (kind >= IndirectTruncate && range()) // no more curly braces @@ +2319,5 @@ > } > > MDefinition::TruncateKind > +MNop1::operandTruncateKind(size_t index) const > +{ What about adding MOZ_ASSERT(index == 0);

(In reply to Benjamin Bouvier [:bbouvier] from comment #5) > @@ +990,5 @@ > > + static MNop1 *NewRestrictTruncate(TempAllocator &alloc, MDefinition *input, TruncateKind kind) { > > + return new(alloc) MNop1(input, kind); > > + } > > + > > + MDefinition *input() { > > nit: this method is already present in MUnaryInstruction, modulo constness. > There's a risk of hidding the original method. Doesn't gcc complain about it? gcc did not complain, or I did not noticed it. > @@ +2319,5 @@ > > } > > > > MDefinition::TruncateKind > > +MNop1::operandTruncateKind(size_t index) const > > +{ > > What about adding MOZ_ASSERT(index == 0); I do not see any need for this assertion.

https://hg.mozilla.org/integration/mozilla-inbound/rev/59d8d82211f2 https://tbpl.mozilla.org/?tree=Try&showall=1&rev=5807cfd4d171