function g() { "use asm"; function f() { var infinity = 0.0; var three = 0.0; var nan = 0.0; var result = 0; infinity = 1.0 / 0.0; three = 1.0 + 2.0; nan = infinity % three; result = ~~(nan + 42.0); return result | 0; } return f; } print(g()()); asm.js result: 42 normal result: 0

I'm fairly certain this is an Ion range anslysis bug: the expression ~~(nan + 42.0) turns into the MIR MTruncateToInt32(MAdd(nan, MConstant(42.0))) and then AdjustTruncatedInputs is converting this into MAdd(MTruncateToInt32(nan), MTruncateToInt32(MConstant(42.0))) The effect is that we turn (NaN+42)|0 into (NaN|0)+42, which is invalid. I'm having trouble getting Ion to take the same path, but I don't see what Odin could be doing wrong here unless there is some flag on MAdd or MTruncateToInt32 nodes that is being incorrectly set that is lying to the truncation analysis. hg annotate seems to indicate that nbp or hannes would be the best ones to ask about this.

The previous test case doesn't happen with Ion as apparently, constant folding is directly applied and the final result is directly computed. With asm.js, constant folding stops at 1.0 + 2.0, as it results in 3, which is an integer and would change the type of the resulting constant (from double to int). I wonder why this doesn't happen with Ion. Here is a test case that shows up with Ion, but not with Odin (do not launch with --ion--eager). I think that it's the same problem, though. It just replaces the value of 1.0 by a variable which is given at call, avoiding the constant propagation. function g() { function f(v) { v = +v; var infinity = 0.0; var three = 0.0; var nan = 0.; var result = 0; infinity = 1.0 / 0.0; three = v + 2.0; nan = infinity % three; result = ~~(nan + 42.0); return result | 0; } return f } g = g() var x; for(i=0; i < 20000; ++i) { x = g(1.0) } print(x) // prints 42, not 0

Created attachment 761237 [details] [diff] [review] proposed fix All tests run with this one. Infinity % 3 == NaN, so the range analysis interval should be the same as the one produced by NaN, which is the empty range (as discussed with h4writer on IRC). Brian, I asked you for review as |hg annotate| shows me that you've written part of this function (nbp is currently on PTO), but if you think somebody else should review this code, feel free to change the requestee.

Created attachment 761249 [details] [diff] [review] proposed fix, even simpler As h4writer noticed, it's even simpler this way and has the same meaning. (See previous comment for further details)

Comment on attachment 761249 [details] [diff] [review] proposed fix, even simpler Review of attachment 761249 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/RangeAnalysis.cpp @@ +728,5 @@ > Range rhs(getOperand(1)); > + > + // Infinite % x is NaN > + if (lhs.isInfinite()) { > + setRange(new Range()); I don't think the setRange call is actually necessary, as if no range is set for an MDefinition it is treated as if it had an infinite range (see Range::Range(MDefinition*))

https://hg.mozilla.org/integration/mozilla-inbound/rev/31efb8905476

https://hg.mozilla.org/mozilla-central/rev/31efb8905476