Last Comment Bug 878444 - IonMonkey: different result after truncation hoisting in range analysis
: IonMonkey: different result after truncation hoisting in range analysis
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla24
Assigned To: Benjamin Bouvier [:bbouvier]
:
:
Mentors:
Depends on:
Blocks: jsfunfuzz
  Show dependency treegraph
 
Reported: 2013-06-01 06:41 PDT by Jesse Ruderman
Modified: 2013-06-14 08:40 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (1.60 KB, patch)
2013-06-11 17:32 PDT, Benjamin Bouvier [:bbouvier]
no flags Details | Diff | Splinter Review
proposed fix, even simpler (1.59 KB, patch)
2013-06-11 18:05 PDT, Benjamin Bouvier [:bbouvier]
bhackett1024: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2013-06-01 06:41:09 PDT
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] 2013-06-08 02:22:24 PDT
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] 2013-06-11 16:56:32 PDT
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] 2013-06-11 17:32:45 PDT
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.
Comment 4 Benjamin Bouvier [:bbouvier] 2013-06-11 18:05:49 PDT
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 5 Brian Hackett (:bhackett) 2013-06-12 07:08:26 PDT
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] 2013-06-12 12:20:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/31efb8905476
Comment 7 Ed Morley [:emorley] 2013-06-13 02:35:57 PDT
https://hg.mozilla.org/mozilla-central/rev/31efb8905476

Note You need to log in before you can comment on or make changes to this bug.