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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: djvj)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Other Branch
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jsbugmon:update,ignore])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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)

Updated

5 years ago
Assignee: general → kvijayan
(Assignee)

Comment 3

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

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

5 years ago
Created attachment 624509 [details] [diff] [review]
Patch 2

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

5 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

5 years ago
You are correct :)

Changed, committed, and pushed:
https://hg.mozilla.org/projects/ionmonkey/rev/14fcd7cb0ba7
(Assignee)

Comment 9

5 years ago
Fixed syntax issue:
https://hg.mozilla.org/projects/ionmonkey/rev/d114473c2eab
(Reporter)

Comment 10

5 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 8c54899dae82).
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 11

4 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.