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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: adrake, Assigned: dvander)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Reporter

Description

8 years ago
Posted file Test case
Attached test case asserts on x86 debug builds.
Posted 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
Status: NEW → ASSIGNED
Attachment #550931 - Flags: review?(rpearl)
Comment on attachment 550931 [details] [diff] [review]
fix

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: http://hg.mozilla.org/projects/ionmonkey/rev/b052c9efbc28
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.