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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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);

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

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.

Comment 3

[Sean Stangl [: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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

pushed w/ nits fixed: http://hg.mozilla.org/projects/ionmonkey/rev/74080c02eedc

Comment 5

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug732862.js.