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

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine: JIT
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sunfish, Assigned: nbp)

Tracking

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.
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.
(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).
(Reporter)

Comment 3

4 years ago
But 1080991 is another place where this surfaced.
Created attachment 8504799 [details] [diff] [review]
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.
Attachment #8504799 - Flags: review?(shu)
Created attachment 8504801 [details] [diff] [review]
Assert that no operands are discarded.
Attachment #8504801 - Flags: review?(sunfish)

Comment 6

4 years ago
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.
Attachment #8504799 - Flags: review?(shu) → review+
(Reporter)

Comment 7

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

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

\o/
Attachment #8504801 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/mozilla-central/rev/7d18f72c70f9
https://hg.mozilla.org/mozilla-central/rev/2a2a1889250e
Assignee: nobody → nicolas.b.pierron
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1083681
You need to log in before you can comment on or make changes to this bug.