878444 - IonMonkey: different result after truncation hoisting in range analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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

Comment 1

[Luke Wagner [:luke]]

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.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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 5

[Brian Hackett [Laid off!]]

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*))

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 7

[Ed Morley [:emorley]]

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