Closed Bug 754719 Opened 8 years ago Closed 8 years ago

IonMonkey: Assertion failure: ins->lhs()->type() == MIRType_Int32, at ion/Lowering.cpp:659


(Core :: JavaScript Engine, defect, major)

Other Branch
Not set





(Reporter: decoder, Assigned: djvj)


(Blocks 1 open bug)


(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update,ignore])


(1 file, 1 obsolete file)

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) {}
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.
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: general → kvijayan
Attached patch Patch (obsolete) — Splinter Review
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)
Attached patch Patch 2Splinter Review
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+
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)
You are correct :)

Changed, committed, and pushed:
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 8c54899dae82).
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Closed: 8 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.