Last Comment Bug 754719 - IonMonkey: Assertion failure: ins->lhs()->type() == MIRType_Int32, at ion/Lowering.cpp:659
: IonMonkey: Assertion failure: ins->lhs()->type() == MIRType_Int32, at ion/Low...
Status: RESOLVED FIXED
[jsbugmon:update,ignore]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: Kannan Vijayan [:djvj]
:
:
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-05-13 15:19 PDT by Christian Holler (:decoder)
Modified: 2013-02-07 05:13 PST (History)
8 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.00 KB, patch)
2012-05-16 11:17 PDT, Kannan Vijayan [:djvj]
no flags Details | Diff | Splinter Review
Patch 2 (1.80 KB, patch)
2012-05-16 13:30 PDT, Kannan Vijayan [:djvj]
dvander: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-05-13 15:19:45 PDT
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 Marty Rosenberg [:mjrosenb] 2012-05-14 10:34:08 PDT
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 Marty Rosenberg [:mjrosenb] 2012-05-14 10:41:19 PDT
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.
Comment 3 Kannan Vijayan [:djvj] 2012-05-16 11:17:50 PDT
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.
Comment 4 Kannan Vijayan [:djvj] 2012-05-16 13:30:57 PDT
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.
Comment 5 David Anderson [:dvander] 2012-05-16 19:17:14 PDT
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.
Comment 6 Kannan Vijayan [:djvj] 2012-05-17 07:45:01 PDT
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).
Comment 7 David Anderson [:dvander] 2012-05-17 11:09:11 PDT
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)
Comment 8 Kannan Vijayan [:djvj] 2012-05-17 11:47:12 PDT
You are correct :)

Changed, committed, and pushed:
https://hg.mozilla.org/projects/ionmonkey/rev/14fcd7cb0ba7
Comment 9 Kannan Vijayan [:djvj] 2012-05-17 12:05:01 PDT
Fixed syntax issue:
https://hg.mozilla.org/projects/ionmonkey/rev/d114473c2eab
Comment 10 Christian Holler (:decoder) 2012-05-18 03:40:52 PDT
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 8c54899dae82).
Comment 11 Christian Holler (:decoder) 2013-02-07 05:13:32 PST
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397

Note You need to log in before you can comment on or make changes to this bug.