IonMonkey: different result after truncation hoisting in range analysis

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: bbouvier)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
mozilla24
x86_64
Mac OS X
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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

4 years ago
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.
No longer blocks: 840284
Summary: OdinMonkey: Odd result with (double % double) nan → IonMonkey: different result after truncation hoisting in range analysis
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Comment 3

4 years ago
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.
Assignee: general → bbouvier
Status: NEW → ASSIGNED
Attachment #761237 - Flags: review?(bhackett1024)
(Assignee)

Comment 4

4 years ago
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)
Attachment #761237 - Attachment is obsolete: true
Attachment #761237 - Flags: review?(bhackett1024)
Attachment #761249 - Flags: review?(bhackett1024)
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*))
Attachment #761249 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/31efb8905476
https://hg.mozilla.org/mozilla-central/rev/31efb8905476
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.