Closed Bug 939868 Opened 6 years ago Closed 6 years ago

IonMonkey: Incorrect result for (x || Math.fround(y))

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jruderman, Assigned: bbouvier)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase)

Attachments

(1 file)

function f(x, y) { return x || Math.fround(y); }
assertEq(f(0, 0), 0);
assertEq(f(0xfffffff, 0), 0xfffffff);

With --ion-eager, I get:
Assertion failed: got 268435456, expected 268435455

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/8621bdc40841
user:        Benjamin Bouvier
date:        Wed Sep 11 02:10:17 2013 -0700
summary:     Bug 900257: Inline Math.fround in IonMonkey; r=sstangl
268435456 is the closest Float32 value from 268435455, so I would guess the phi type of the condition is not correct, either in MergeTypes or the ApplyTypes pass. Investigating.
Attached patch Patch + testsSplinter Review
Very nice find!

This could happen because int32 and float32 inputs for a phi would be unconditionally merged into float32. We have to actually check that the int32 value can produce a float32, so that we can merge into a float32 type (e.g. for constants, it means that the constant is precisely representable as a float32).

As this could also happen when propagating phi specializations, I also fixed it there and added a few test cases that were failing either in normal mode or ion-eager mode.
Assignee: nobody → benj
Status: NEW → ASSIGNED
Attachment #8334830 - Flags: review?(sstangl)
Comment on attachment 8334830 [details] [diff] [review]
Patch + tests

Review of attachment 8334830 [details] [diff] [review]:
-----------------------------------------------------------------

Nice fix. Does this affect tracked benchmarks? Were we improperly consuming integers?

::: js/src/jit/IonAnalysis.cpp
@@ +508,5 @@
>          }
>          if (use->type() != phi->type()) {
> +            // Specialize phis with int32 that can be converted to float and float operands as floats.
> +            if ((use->type() == MIRType_Int32 && use->canProduceFloat32() && phi->type() == MIRType_Float32)
> +                || (phi->type() == MIRType_Int32 && phi->canProduceFloat32() && use->type() == MIRType_Float32))

Prefer "||" on above line, with "use->" and "phi->" aligned.
Attachment #8334830 - Flags: review?(sstangl) → review+
No impact on any of the benchmarks mentioned in the blog post (they were only using small constants).

https://hg.mozilla.org/integration/mozilla-inbound/rev/13e33fcc873d
https://hg.mozilla.org/mozilla-central/rev/13e33fcc873d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.