IonMonkey: "Assertion failure: !mul->canBeNegativeZero() && !mul->canOverflow()" with imul

RESOLVED FIXED in Firefox 24

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: h4writer)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla25
x86_64
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox23 wontfix, firefox24 fixed, firefox25 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
./js --ion-eager

function f(x)
{
  if (Math.imul(x, 0xffffffff)) {
    return -x;
  }
  return 1;
}
f(0);
f(0);

Assertion failure: !mul->canBeNegativeZero() && !mul->canOverflow(), at js/src/ion/shared/CodeGenerator-x86-shared.cpp:5
(Reporter)

Comment 1

5 years ago
This goes back to January if the arguments to imul are swapped:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/b9c4a9483492
user:        Tom Schuster
date:        Fri Jan 04 00:10:19 2013 +0100
summary:     Bug 822436 - IonMonkey: Inline Math.imul. r=h4writer
Blocks: 822436
Keywords: regression
(Reporter)

Comment 2

5 years ago
(The testcase in comment 0 only goes back to April, with bug 864402.)
(Assignee)

Comment 3

5 years ago
The assertion is added for certainty that Math.imul got all the performance speedups. So failing that assert doesn't result in any errors, except performance degradation. I'll look at it after finishing sec bugs. Evilpie feel free to steal, though ;).
(Reporter)

Updated

5 years ago
Assignee: jhpedemonte → general
Component: Java to XPCOM Bridge → JavaScript Engine
(Assignee)

Comment 4

5 years ago
I cannot repro this on x86 debug build on b9c4a9483492, 7037549c9fbb nor tip, using --ion-eager. What am I missing here so I can repro? Extra compiler flags?
Flags: needinfo?(jruderman)
(Reporter)

Comment 5

5 years ago
Dunno.  I can reproduce without any special compiler flags, with both 32-bit and 64-bit builds.
Flags: needinfo?(jruderman)
(Assignee)

Comment 6

5 years ago
Created attachment 767670 [details] [diff] [review]
Patch

No idea why I couldn't repro yesterday.
We aren't looking to the mode when testing for congruence and as a result we merge the two multiplications.
Assignee: general → hv1989
Attachment #767670 - Flags: review?(evilpies)
(Assignee)

Updated

5 years ago
Attachment #767670 - Attachment is patch: true
(Assignee)

Comment 7

5 years ago
Created attachment 767734 [details] [diff] [review]
Patch

Both patches are correct, but this will do the merge. I just reasoned that merging will create better code.
Attachment #767670 - Attachment is obsolete: true
Attachment #767670 - Flags: review?(evilpies)
Attachment #767734 - Flags: review?(evilpies)
Comment on attachment 767734 [details] [diff] [review]
Patch

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

This patch is indeed nicer.
Attachment #767734 - Flags: review?(evilpies) → review+
Looks like this regressed several asm.js benchmarks on awfy: zlib (18% on Odin) and primes (7% slowdown on non-Odin).
(Assignee)

Comment 11

5 years ago
(In reply to Alon Zakai (:azakai) from comment #10)
> Looks like this regressed several asm.js benchmarks on awfy: zlib (18% on
> Odin) and primes (7% slowdown on non-Odin).

Oh really! This shouldn't have made a difference to what we did. I'll backout tomorrow, check the other fix and push that.
(Assignee)

Comment 12

5 years ago
(In reply to Hannes Verschore [:h4writer] from comment #11)
> (In reply to Alon Zakai (:azakai) from comment #10)
> > Looks like this regressed several asm.js benchmarks on awfy: zlib (18% on
> > Odin) and primes (7% slowdown on non-Odin).
> 
> Oh really! This shouldn't have made a difference to what we did. I'll
> backout tomorrow, check the other fix and push that.

On the other hand, awfy was having problems and wasn't updating. I fixed that around that hour. That means all improvements/regressions since GGC started logging are listed on that revision! I'll bisect manually to find the real regressor.
https://hg.mozilla.org/mozilla-central/rev/c544e1904389
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Comment 15

5 years ago
Comment on attachment 767734 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 822436 (FF20+)

User impact if declined: Only debug builds will assert in a corner case (it took the fuzzers till now to come up with the issue). Release builds are fine.

Testing completed (on m-c, etc.): m-i / m-c: 1 day

Risk to taking this patch (and alternatives if risky): No risk. It actually just removes an Assert. The mode_ variable is only read by the assert.

String or IDL/UUID changes made by this patch: /
Attachment #767734 - Flags: approval-mozilla-beta?
Attachment #767734 - Flags: approval-mozilla-aurora?
Comment on attachment 767734 [details] [diff] [review]
Patch

We see no significant user benefit to uplifting, but we're early enough in the cycle to uplift to Aurora.
Attachment #767734 - Flags: approval-mozilla-beta?
Attachment #767734 - Flags: approval-mozilla-beta-
Attachment #767734 - Flags: approval-mozilla-aurora?
Attachment #767734 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e520729e67c5
status-firefox24: --- → fixed
status-firefox25: --- → fixed
status-firefox23: --- → wontfix
You need to log in before you can comment on or make changes to this bug.