1055690 - IonMonkey: Don't leave discarded defs sitting around in resume point operands

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

While working on bug 1029830, I tried to add an assert that no MNode has an operand with the isDiscarded() flag set in AssertBasicGraphCoherency, however it fails. At least one place that triggers this is EliminatePhis, which detects when resume points have unobserved operands and discards them without releasing the actual operands.

It would be nice to clean up all such places so that we can assert that instructions don't have discarded operands.

Comment 1

[Nicolas B. Pierron [:nbp]]

I have patches to clean the status of resume points, but for the moment I am trying to reduce a test case to attach it with the patch to make sure nobody breaks this clean-up anymore.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #0)
> While working on bug 1029830, I tried to add an assert that no MNode has an
> operand with the isDiscarded() flag set in AssertBasicGraphCoherency,
> however it fails. At least one place that triggers this is EliminatePhis,
> which detects when resume points have unobserved operands and discards them
> without releasing the actual operands.

Ok, I miss understood the first time.  The EliminatePhis is supposed to replace the operand by a constant such as Magic(OpitmizedOut).

Comment 3

[Dan Gohman [:sunfish]]

But 1080991 is another place where this surfaced.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Do not Transplant resume points on MBail instructions.

MBail is an instruction which is doing a bailout.  When we lower
instructions, we build a snapshot based on the previous resume point, and
not the one which is currently on the instruction.

The resume point which is attached to an instruction is used to resume after
the current instruction, and most of the time it holds the return value of
the instruction on which it is attached.

So, in addition to be unused, as there is no more instruction which can
bailout after the MBail instruction, the resume point kept a reference to
the old instruction on which it were located, which is being discarded after
doing the transplant of the resume point.

This patch removes the resume point transplant on MBail.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Assert that no operands are discarded.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8504799 [details] [diff] [review]
Do not Transplant resume points on MBail instructions.

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

::: js/src/jit/ParallelSafetyAnalysis.cpp
@@ +455,5 @@
>      MOZ_ASSERT(block->isMarked()); // `block` must have been reachable to get here
>  
>      clearUnsafe();
>  
>      // Allocate a new bailout instruction and transplant the resume point.

Nit: update this comment please.

Comment 7

[Dan Gohman [:sunfish]]

Comment on attachment 8504801 [details] [diff] [review]
Assert that no operands are discarded.

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

\o/

Comment 8

[Nicolas B. Pierron [:nbp]]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2f2d9e4afd63

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d18f72c70f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2a1889250e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d18f72c70f9
https://hg.mozilla.org/mozilla-central/rev/2a2a1889250e