865153 - IonMonkey: RestartLoop isn't done correctly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Attachment: Patch to assert upon fault

Decoder found that we were looking to values on MBasicBlocks that weren't set on the following testcase:

function testMonitorIntrinsic() {
  var N = 2;
  var p = new ParallelArray([N,N], function () 0);
  for (var i = 0; i < 3; i++) {
    for (var j = 0; j < 2; j++) {
      p.get(i).get(j);
    }
  }
}
testMonitorIntrinsic();

The issue is when compiling self-hosted:2909 in IM we need to restart the loop. When restarting a loop, the information about blocks isn't totally wiped out. And some of the supposedly removed blocks remain accessible!

In this case the uses of the phis in the loopheader aren't removed!

I've added a simple patch to assert when this happens. To make it easier to spot when this bug is fixed...

Comment 1

[Brian Hackett [Laid off!]]

All instructions in the loop header's block, and all instructions and phis in removed blocks should be removed by RestartLoop, which is supposed to remove all the uses of the loop header phis.  Which uses are remaining?

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

This removes resume points when marking blocks dead, in addition to phis and instructions as are already done.  Adds bookkeeping to keep track of resume points in blocks, which was not previously done.  I weakened the assertion in domIndex() as it is firing on an unrelated jit-test (basic/bug666448.js) which also presumably has unreachable blocks not handled by the dominator analysis.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 741618 [details] [diff] [review]
patch

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

Nice! Shouldn't need a second review, but just answer my question and add the r? flag again.

::: js/src/ion/IonBuilder.cpp
@@ +1746,2 @@
>      header->discardAllInstructions();
> +    header->discardAllResumePoints(false);

Add /* discardEntry */ in front of false

::: js/src/ion/MIRGraph.cpp
@@ +207,5 @@
>      info_(info),
>      stackPosition_(info_.firstStackSlot()),
>      lastIns_(NULL),
>      pc_(pc),
> +    domIndex_(uint32_t(-1)),

This can get removed, since you use markAsDead and asserts on !isDead()

@@ +300,1 @@
>      }

I don't really get what this does and why? When would we want to have an entry resumepoint that has a parent resumepoint but all it's operands are cleared?

@@ +644,5 @@
>  }
>  
>  void
> +MBasicBlock::discardAllResumePoints(bool discardEntry)
> +{

I think it would make sense to default this to "true"

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Hannes Verschore [:h4writer] from comment #3)
> I don't really get what this does and why? When would we want to have an
> entry resumepoint that has a parent resumepoint but all it's operands are
> cleared?

The entry resume point here has not had its operands initialized, and the caller is not guaranteed to initialize them properly (and in at least one case will not).  Having the operands initialized is necessary for walking the list of resume points and removing the uses of them, as otherwise that walk will read garbage.

Comment 5

[Hannes Verschore [:h4writer]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> The entry resume point here has not had its operands initialized, and the
> caller is not guaranteed to initialize them properly (and in at least one
> case will not).  Having the operands initialized is necessary for walking
> the list of resume points and removing the uses of them, as otherwise that
> walk will read garbage.

Could you add a little explanation in the code about that?

Comment 6

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb9d7a408ab

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eeb9d7a408ab