This test fails, returning 0 from f(1) instead of 1: function f(x) { return Math.round((x >>> 0) / 2) >> 0; } assertEq(f(1), 1); This suggests a bug in the JIT implementation of integer division. Source: Mozilla-inbound from September 7, plus some patches that should have no bearing on this. Platform: Quad 2.3GHz Cortex-A15 NVIDIA "Jetson" TK1 dev board, Ubuntu 14.04.

(Or in the JIT implementation of round, of course.)

Actually this is required: function f(x) { return Math.round((x >>> 0) / 2) >> 0; } f(2); assertEq(f(1), 1); Bug 1122401 points to Math.round as a troublespot.

Some sort of interaction between baseline and ion? With ion enabled and --no-baseline it succeeds; with ion disabled and --baseline-eager it succeeds. Only with --ion-eager --no-threads does it fail. In that case there is only one call to js::math_round (from baseline), the second call (that occurs in the other two cases) is optimized out along the way. The test case works properly in the simulator.

Tentatively this is related to the use of UDiv on ARM. UDiv is generated as an optimization - don't know if that is right or wrong, yet - but it does not bail out if the result is not representable as an integer, even though that appears to be the expectation of the calling code. In this case we have 1/2 yielding zero as the result but round(0.5) = 1 so it's not good enough to stop there. UDiv is generated as the optimization for the failing invocation of f() in the test case, but not for the succeeding one - in that case, the lowering code falls through to correct divide-by-constant code.

(In reply to Lars T Hansen [:lth] from comment #3) > > The test case works properly in the simulator. That is because the simulator takes the soft-divide path, which is because the capability idiva is not set by default. Once idiva is enabled on the simulator then the simulator run fails too. I'm not sure if that default for idiva is good or bad. Support for UDIV and SDIV is tied not to VFP, but to the Virtualization Extensions; whether UDIV and SDIV are implemented on systems without the Virtualization Extensions is implementation defined. Cortex-A7 and Cortex-A15 appear to have the Virtualization Extension normally, The Cortex-A9 probably does not (but might still have the instructions).

The issue is probably semi-known, see bug 1122401 comment 20 and comments before/after: looks like the use of hardware divide on ARM has some half-baked bits.

Probable fix, but I want to look for similar cases before landing.

Comment on attachment 8660410 [details] [diff] [review] bug1202643-udiv-bailout.patch r? to nbp due to his involvement with bug 1122401, see earlier comments. (Reasonable people could disagree on whether to use mls(), cmp(0) as I have done or mul(), cmp(lhs).)

Comment on attachment 8660410 [details] [diff] [review] bug1202643-udiv-bailout.patch Review of attachment 8660410 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm/CodeGenerator-arm.cpp @@ +2120,2 @@ > if (!ins->mir()->isTruncated()) { > + MOZ_ASSERT(ins->mir()->fallible()); Doesn't the fact that we have a snapshot implies that this instruction is fallible? @@ +2128,5 @@ > + MOZ_ASSERT(ins->mir()->fallible()); > + { > + ScratchRegisterScope scratch(masm); > + masm.as_mls(scratch, rhs, output, lhs); > + masm.ma_cmp(scratch, Imm32(0)); Indeed, as_mul, might be better in a few cases. MLS takes one extra cycle compare the to SUB, and the compare instruction does not seems to make exceptions of zero.

(In reply to Nicolas B. Pierron [:nbp] from comment #9) > Comment on attachment 8660410 [details] [diff] [review] > bug1202643-udiv-bailout.patch > > Review of attachment 8660410 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/arm/CodeGenerator-arm.cpp > @@ +2120,2 @@ > > if (!ins->mir()->isTruncated()) { > > + MOZ_ASSERT(ins->mir()->fallible()); > > Doesn't the fact that we have a snapshot implies that this instruction is > fallible? You have a point, but I'm not sure there's any programmatic connection between those facts. fallible() tests a flag in the MIR node and snapshot() gets a pointer out of the LIR node. (We'll run into an NPE shortly thereafter if the pointer is not set because there's no shapshot.) In truth I just duplicated this assertion from elsewhere because it seemed sensible, but I'm not wedded to it.