Closed Bug 674694 Opened 10 years ago Closed 10 years ago

IonMonkey: Assertion failure: GetEffectiveType(in) == phi->type(), at IonAnalysis.cpp:331


(Core :: JavaScript Engine, defect)

Not set





(Reporter: adrake, Assigned: dvander)


(Blocks 1 open bug)



(2 files)

Attached file Test case
Attached test case asserts on x86 debug builds.
Attached patch fixSplinter Review
There were a number of bugs here. The overall bug is that phi specialization did not reflow properly at all, and MInstruction::useAsType was never called. Patch summary:

 * There is now a TypePolicy callback for calling useAsType. If this changes
   an untyped node's observed type, its phi uses are re-analyzed.
 * Phis now participate in the initial worklist.
 * A bug where MUseDefIterator skipped the first use has been fixed.
 * A bug where LICM returned OOM prematurely has been fixed.
 * The TypeAnalyzer is now mostly infallible.
 * A big comment explaining the algorithm has been added.
 * A useless callback in TypePolicy has been removed.
Assignee: general → dvander
Attachment #550931 - Flags: review?(rpearl)
Comment on attachment 550931 [details] [diff] [review]

Review of attachment 550931 [details] [diff] [review]:

New logic looks good, and is more clear than the old version.

A few very minor issues.

::: js/src/ion/IonAnalysis.cpp
@@ +134,1 @@
>      bool push(MDefinition *def) {

|push()| is only infallable after the initial push. If push cannot fail, since all instructions have already been added to the vector, and thus the vector is guaranteed to have space, then there should be an infallible push function, so that we can assert that it doesn't fail.

::: js/src/ion/LICM.cpp
@@ +172,5 @@
>                  // if the consumer of this invariant instruction is in the
>                  // loop, and it is also worth hoisting, then process it.
>                  if (isInLoop(consumer) && isHoistable(consumer)) {
> +                    if (!insertInWorklist(consumer->toInstruction()))

is this really part of this bug's patch?

::: js/src/jit-test/tests/ion/bug674694.js
@@ +57,5 @@
> +        v5 = v1;
> +    }
> +    v6 = (v4 & (v5 + v5));
> +}
> +f0(7517,3991,2617,5405);

Add a comment that the goal of this test is to compile without asserting, or maybe add an assert for the correct value.

And the test-case should probably be simplified to something close to a minimal one?
Attachment #550931 - Flags: review?(rpearl) → review+
Pushed w/ nits:
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.