1040940 - Remove resume point of discarded instructions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

When we use block->discardAt(ins), we do remove all the uses of the operand of the instruction, but we do not remove all the uses of the operands of the resume point, as well as the resume point.

It would be sane to discard the resume points at the same time such as we can avoid having dangling resume points in the list of resume points of the basic blocks.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Discard resume point with discarded instructions.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8458909 [details] [diff] [review]
Discard resume point with discarded instructions.

(load balancing reviews)

Comment 3

[Dan Gohman [:sunfish]]

Comment on attachment 8458909 [details] [diff] [review]
Discard resume point with discarded instructions.

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

::: js/src/jit/MIRGraph.cpp
@@ +734,5 @@
> +        MResumePointIterator iter = resumePointsBegin();
> +        while (*iter != rp)
> +            iter++;
> +        resumePoints_.removeAt(iter);
> +    }

We should update the MInstructionReverseIterator version of MBasicBlock::discardAt and MBasicBlock::discard too, as they should be kept consistent. It'd be nice to have this factored so that we only have one copy of the actual code.

Hrm, also MBasicBlock::discardAllInstructionsStartingAt. It should probably just call discardAt ion each instruction instead of handling them manually.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Discard resume point with discarded instructions.

Delta:
 - Use the same function to discard any/all instructions.

Comment 5

[Dan Gohman [:sunfish]]

Comment on attachment 8461473 [details] [diff] [review]
Discard resume point with discarded instructions.

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

::: js/src/jit/IonAnalysis.cpp
@@ +1742,5 @@
>                          JS_ASSERT(ins->block() == iter->block());
> +
> +                    for (uint32_t i = 0, e = resume->numOperands(); i < e; i++) {
> +                        MOZ_ASSERT(resume->getUseFor(i)->hasProducer());
> +                    }

This for loop doesn't need braces.

::: js/src/jit/MIR.h
@@ +93,5 @@
>       * instructions.
>       */                                                                         \
> +    _(RecoveredOnBailout)                                                       \
> +                                                                                \
> +    /* The current instruction got discarded from the MIR Graph. This useful

"This useful" -> "This is useful"

@@ +94,5 @@
>       */                                                                         \
> +    _(RecoveredOnBailout)                                                       \
> +                                                                                \
> +    /* The current instruction got discarded from the MIR Graph. This useful
> +     * when we want to iterate over resume points ans instructions, while

"ans" -> "and"

@@ +95,5 @@
> +    _(RecoveredOnBailout)                                                       \
> +                                                                                \
> +    /* The current instruction got discarded from the MIR Graph. This useful
> +     * when we want to iterate over resume points ans instructions, while
> +     * handling instructions which are without reporting to the iterator.

"which are without" -> "which are discarded without"

@@ +775,5 @@
>      void setResumePoint(MResumePoint *resumePoint) {
>          JS_ASSERT(!resumePoint_);
>          resumePoint_ = resumePoint;
>      }
> +    // Used to transfert the resume point to the rewritten instruction.

"transfert" -> "transfer"

::: js/src/jit/MIRGraph.cpp
@@ +727,3 @@
>      // Instructions captured by resume points cannot be safely discarded, since
>      // they are necessary for interpreter frame reconstruction in case of bailout.
> +    MOZ_ASSERT_IF(refType & RefType_AssertNoUses, !ins->hasUses());

Since this assert is a precondition of the function, and not a postcondition of the resume point removal code, it should happen at the top of the function, for clarity.

::: js/src/jit/MIRGraph.h
@@ +62,5 @@
>      void setVariable(uint32_t slot);
>  
> +    enum ReferencesType {
> +        RefType_AssertNoUses = 1 << 0,
> +        RefType_DiscardOperands = 1 << 1,

I'm confused about the purpose of RefType_DiscardOperands. It's actually asserted to always be set. Are you anticipating this will change?

@@ +71,5 @@
> +    // Remove all references to an instruction such that it can be removed from
> +    // the list of instruction, without keeping any dangling pointer to it. This
> +    // includes the operands of the instruction, and the resume point if
> +    // present.
> +    void discardReferencesTo(MInstruction *ins, ReferencesType refType = RefType_Default);

I suggest renaming this to prepareForDiscard, since it does more than just discard references to |ins|.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #5)
> ::: js/src/jit/MIRGraph.cpp
> @@ +727,3 @@
> >      // Instructions captured by resume points cannot be safely discarded, since
> >      // they are necessary for interpreter frame reconstruction in case of bailout.
> > +    MOZ_ASSERT_IF(refType & RefType_AssertNoUses, !ins->hasUses());
> 
> Since this assert is a precondition of the function, and not a postcondition
> of the resume point removal code, it should happen at the top of the
> function, for clarity.

This is what I thought at the beginning but this is not true.  If we are removing instruction which are effectful, and thus having a resume point, then the resume point might use the instruction which is holding it.  So we have to discard the resume point uses before checking for other uses.

> ::: js/src/jit/MIRGraph.h
> @@ +62,5 @@
> >      void setVariable(uint32_t slot);
> >  
> > +    enum ReferencesType {
> > +        RefType_AssertNoUses = 1 << 0,
> > +        RefType_DiscardOperands = 1 << 1,
> 
> I'm confused about the purpose of RefType_DiscardOperands. It's actually
> asserted to always be set. Are you anticipating this will change?

I just wanted to name what the function was doing.  But I do make use of this flag in Bug 1042729 part 3, and check if we need to discard the resume point operands when we discard a resume point.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Discard resume point with discarded instructions.

Delta:
 - Apply comment modifications.
 - Rename discardReferencesTo to prepareForDiscard.
 - Update the comment above MOZ_ASSERT_IF(..., !ins->hasUses()).

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8462574 [details] [diff] [review]
Discard resume point with discarded instructions.

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

Ah, sounds good then. Thanks for adding that comment.

Comment 9

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a1c841c6b3b0
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8a746c3e25be

https://hg.mozilla.org/integration/mozilla-inbound/rev/67550c8c9546

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67550c8c9546