Closed
Bug 754719
Opened 12 years ago
Closed 12 years ago
IonMonkey: Assertion failure: ins->lhs()->type() == MIRType_Int32, at ion/Lowering.cpp:659
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: decoder, Assigned: djvj)
References
Details
(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update,ignore])
Attachments
(1 file, 1 obsolete file)
1.80 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
The following testcase asserts on ionmonkey revision e8de64e7e9fe (run with --ion -n -m): function TestCase(n, d, e, a) {} (function( ) { for (var i = 0; i < 64; ++i) { switch (~~(TestCase) % 3) {} } })();
Comment 1•12 years ago
|
||
A quick look into this shows that --ion-gvn=off fixes it. My hunch is that ~~TestCase is being replaced with TestCase which either has no type information, or just a different type altogether.
Comment 2•12 years ago
|
||
And this isn't GVN's fault per se, it is the generic constant folding code that we have, which only gets executed during GVN.
Assignee | ||
Updated•12 years ago
|
Assignee: general → kvijayan
Assignee | ||
Comment 3•12 years ago
|
||
As mjrosenb surmised, this is due to GVN folding just eliding the two bitwise nots and using the original expression directly, but the original expression is not specialized to Int32, so it fails match up to the modulo operation. Also as he noted, this issue also exists in GVN for all the binary bitwise operators, which can sometimes fold to one of their inputs, but it doesn't present in this case. The patch resolves the cases for all bitwise ops, by just checking to see if the folded operation has a type of MIRType_Int32, and if not, wrapping it with a MToInt32.
Attachment #624459 -
Flags: review?(dvander)
Assignee | ||
Comment 4•12 years ago
|
||
The previous assessments were a bit off the mark. The folding methods for binary bitops check specialization, which suffices to ensure that they can use their operands in place of themselves in the appropriate conditions. Updated patch adds asserts to make sure that unexpected MIRTypes are not sneaking into the operands for binary bitwise ops, and otherwise makes a much smaller change to MBitNot::foldsTo - just ensuring that the input is specialized to Int32 before replacing the operation with its input's input.
Attachment #624459 -
Attachment is obsolete: true
Attachment #624459 -
Flags: review?(dvander)
Attachment #624509 -
Flags: review?(dvander)
Comment on attachment 624509 [details] [diff] [review] Patch 2 Review of attachment 624509 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/MIR.cpp @@ +492,5 @@ > > void > MBitNot::infer(const TypeOracle::Unary &u) > { > + JS_ASSERT(u.ival != MIRType_Value && u.ival != MIRType_Any); Any is definitely invalid, but I think the input can be Value. @@ +534,5 @@ > void > MBinaryBitwiseInstruction::infer(const TypeOracle::Binary &b) > { > + JS_ASSERT(b.lhs != MIRType_Value && b.lhs != MIRType_Any); > + JS_ASSERT(b.rhs != MIRType_Value && b.rhs != MIRType_Any); Same here.
Attachment #624509 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Looking over this, it seems that the type policy for bitwise operations will always adjust incoming inputs that don't agree with the instruction's result type (always MIRType_Int32), to do an MTruncateToInt32. The incomings to a bitwise operation, as far as I can tell, is either going to be an Unbox-to-int32, a truncate-to-int32, or the original input itself is typed to int32. I think we should keep the != MIRType_Value assert. I don't see how a Value typed input can sneak through the current analysis, and it would seem to be a bug if it gets to GVN phase without having been adjusted so that the input is either MIRType_Object (causing a LBitNotV to get generated in LIR), or MIRType_Int32 (causing a LBitNotI to get generated in LIR).
The assert will fail: function f(x, y) { var a = x ? { } : "asdf"; return ~a; } f(0, 1); with --ion-eager (Inference happens before type analysis/coercion)
Assignee | ||
Comment 8•12 years ago
|
||
You are correct :) Changed, committed, and pushed: https://hg.mozilla.org/projects/ionmonkey/rev/14fcd7cb0ba7
Assignee | ||
Comment 9•12 years ago
|
||
Fixed syntax issue: https://hg.mozilla.org/projects/ionmonkey/rev/d114473c2eab
Reporter | ||
Comment 10•12 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 8c54899dae82).
Reporter | ||
Updated•12 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 11•11 years ago
|
||
Automatically extracted testcase for this bug was committed: https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•