Closed Bug 732862 Opened 10 years ago Closed 10 years ago

IonMonkey: Assertion failure: slots_[j].def != entryDef, at ion/MIRGraph.cpp:848

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Assigned: dvander)

References

Details

(Keywords: assertion, testcase)

Attachments

(1 file)

The following testcase asserts on ionmonkey revision 1fd6c40d3852 (run with --ion -n -m --ion-eager):


function testReallyDeepNestedExit() {
    for (var i = 0; i < 5*4; i++) {}
    for (var o = schedule = i = 9 ; i < 5; i++) {}
}
assertEq(testReallyDeepNestedExit(), 198);
The problem is the bytecode sequence:
  INT8 9
  SETLOCAL 0
  SETGNAME "schedule"
  SETLOCAL 1
  POP

At the first SETLOCAL, we correctly mark the top of the stack as a copy. However, SETGNAME pops the top of the stack, then re-pushes it, losing the fact that the top was a copy. Now we've leaked the same SSA name into two local variables, which breaks lazy phi placement.

JM dealt with this by using FrameState::shimmy() instead of the explicit pop; pop; push. But I'm getting tired of fixing all these edge cases. Our lazy phi placement algorithm has had many bugs since day one, it places awkward restrictions on MIR building, its edge cases require complex handling (easy 150+ lines of comments alone), blah blah blah. I'm going to see if placing phis eagerly is viable. If it works it should remove a huge amount of code.

A little late to be tearing all this out but better now than in two months.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attached patch patchSplinter Review
No perf change, passes tests. This patch is mostly code removal:

 12 files changed, 70 insertions(+), 481 deletions(-)

The only problem with eager placement is that we generate many more phis. EliminateDeadPhis is now EliminatePhis and has a quick check to remove redundant phis.
Attachment #603937 - Flags: review?(sstangl)
Comment on attachment 603937 [details] [diff] [review]
patch

Review of attachment 603937 [details] [diff] [review]:
-----------------------------------------------------------------

One of my favorite patches.

::: js/src/ion/IonAnalysis.cpp
@@ +137,5 @@
> +            if (MDefinition *redundant = IsPhiRedundant(*iter)) {
> +                iter->replaceAllUsesWith(redundant);
> +                iter = block->discardPhiAt(iter);
> +                continue;
> +            }

Since this block is doing something other than what the comment above suggests, should it have a separate comment?

::: js/src/ion/MIRGraph.h
@@ +69,5 @@
>  };
>  
>  class MBasicBlock : public TempObject, public InlineListNode<MBasicBlock>
>  {
>      static const uint32 NotACopy = uint32(-1);

NotACopy can also be removed.
Attachment #603937 - Flags: review?(sstangl) → review+
pushed w/ nits fixed: http://hg.mozilla.org/projects/ionmonkey/rev/74080c02eedc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug732862.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.