Clean the MBasicBlock list of resume point.

RESOLVED FIXED in mozilla34

Status

()

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

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Currently each basic block keeps a list of resume points which are attached to the basic block or attached to an instruction.  This was done for discarding the uses hold by orphan resume points after the restartLoop phase of IonBuilder (Bug 865153).

With the arrival of the escape analysis and other similar optimizations, we are now able to remove effectful instructions to the extend that we can recover their instructions.  As we remove these instruction we should also remove the resume points which are associated to these instructions (Bug 1040940).

If we do not do so, we might have resume points which are visible on the use list of instructions while not being used for any snapshot during the lowering.  Worse, some instructions seems to be sharing the same resume points, which is unfortunate when we are trying to remove when an instruction which is holding it.

I am opening this bug to add some sanity back to the way we handle the list of resume points attached on the blocks.  We should be able to ensure that the list of resume points only contains valid resume points and that resume points are only attached to one instruction only.
Created attachment 8461487 [details] [diff] [review]
Part 1 - Make resume point unique to each instruction.

This patch adds a MResumePoint::Copy function to replace the implicit reuse
of the MStrart resume point, or the basic block entry resume point.

Doing so will indeed consume more memory, but with the up-comming patches,
we should be able to remove the MInstruction pointer from ResumePoint which
will surely overcome the down-side of allocating more resume point in the
current patch.
Attachment #8461487 - Flags: review?(hv1989)
Created attachment 8461492 [details] [diff] [review]
Part 2 - Ensure that a resume point belongs to one instruction.

Add the graph invariant to make sure that all resume points which are in the
list of resume points are live, and that all instructions are holding their own
resume point if they have one.
Attachment #8461492 - Flags: review?(hv1989)
Attachment #8461492 - Flags: review?(hv1989) → review+
Attachment #8461487 - Flags: review?(hv1989) → review+
Created attachment 8462530 [details] [diff] [review]
Part 3 - Ensure priorResumePoints are re-attached to the new BasicBlock or discarded.

This patch prevent the last case of orphan resume point which are used for
inlining.  This patch do 2 things:
 - Re-attach the resume point to the new block, when the resume point is
 used as the entry point of the new block.
 - Discard the resume point if we do not used it for inlining, and discard
 all its uses.
Attachment #8462530 - Flags: review?(efaustbmo)
Created attachment 8462535 [details] [diff] [review]
Part 4 - Keep a reference to outer resume point on basic blocks, and make discarding resume point precise.

This patch reference precisely the last case of resume point which is not
precisely named.  The outer resume point is used for holding the state of
the stack of Baseline before the call.  Outer resume points are used to be
hold as the caller_ resume point in all the inlined basic blocks.

As this is the last case, we can also remove the processing of the list of
resume point when we are discarding all the resume points, and only assert
that there is no more resume points as soon as we have removed all
instructions.
Attachment #8462535 - Flags: review?(hv1989)
Created attachment 8462536 [details] [diff] [review]
Part 5 - Only list resume points in debug builds.

Move the list of resume points to be a Debug-only thing that we use to
assert that we properly remove blocks, and to verify the graph coherency.
Attachment #8462536 - Flags: review?(hv1989)
Depends on: 992199
Blocks: 992199
No longer depends on: 992199

Comment 6

4 years ago
Comment on attachment 8462530 [details] [diff] [review]
Part 3 - Ensure priorResumePoints are re-attached to the new BasicBlock or discarded.

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

::: js/src/jit/MIRGraph.cpp
@@ +733,5 @@
>      // correctly removing the resume point if any.
>      MOZ_ASSERT(ins->block() == this);
>  
>      MResumePoint *rp = ins->resumePoint();
>      if (refType & RefType_DiscardResumePoint && rp)

nit: pre-existing: man, I wish there was some extra parens here, just for sanity's sake.

::: js/src/jit/MIRGraph.h
@@ +187,5 @@
>          resumePoints_.pushFront(resume);
>      }
>  
> +    // Discard pre-allocated resume point.
> +    void discardPreAllocatedResumePoint(MResumePoint *resume) {

Is there some reason why this function doesn't take a ReferencesType defaulted to RefType_Default, and pass that to discardResumePoint()? That would clean up the caller in NewWithresumePoint a little, maybe. I guess it's up to you.
Attachment #8462530 - Flags: review?(efaustbmo) → review+
Comment on attachment 8462535 [details] [diff] [review]
Part 4 - Keep a reference to outer resume point on basic blocks, and make discarding resume point precise.

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

Doesn't this get tracked with: "MBasicBlock::callerResumePoint()"?
Attachment #8462535 - Flags: review?(hv1989)
Attachment #8462536 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #7)
> Comment on attachment 8462535 [details] [diff] [review]
> Part 4 - Keep a reference to outer resume point on basic blocks, and make
> discarding resume point precise.
> 
> Review of attachment 8462535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Doesn't this get tracked with: "MBasicBlock::callerResumePoint()"?

Yes, the outer resume point is referenced multiple times by all the resume points which are part of the inlined function.

No, and it would not make sense knowing to track it as part of the inline function knowing that the BasicBlock has a CompileInfo which does not correspond to the right function.   So the outerResumePoint block should be located on a basic block which compileInfo tragets the right script otherwise we might miss observable operands of the resume points.
Comment on attachment 8462535 [details] [diff] [review]
Part 4 - Keep a reference to outer resume point on basic blocks, and make discarding resume point precise.

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

Forwarding to Jan, since I'm not certain enough. Looks good or makes sense for me, though.
Attachment #8462535 - Flags: review?(jdemooij)
(In reply to Eric Faust [:efaust] from comment #6)
> > +    // Discard pre-allocated resume point.
> > +    void discardPreAllocatedResumePoint(MResumePoint *resume) {
> 
> Is there some reason why this function doesn't take a ReferencesType
> defaulted to RefType_Default, and pass that to discardResumePoint()? That
> would clean up the caller in NewWithresumePoint a little, maybe. I guess
> it's up to you.

The goal was to make it clear that discarding resume point is not something that you do every day, only when you have pre-allocated it and not yet attached it to any instruction / basic block yet.  Otherwise resume points are discarded when the instruction / basic block is discarded.
Blocks: 1047529
Comment on attachment 8462535 [details] [diff] [review]
Part 4 - Keep a reference to outer resume point on basic blocks, and make discarding resume point precise.

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

::: js/src/jit/MIRGraph.cpp
@@ +797,5 @@
>          // We only discard operands and flag the instruction as discarded as the
>          // resume points are exepected to be removed separately.  Also we do not
>          // assert that we have no uses as block might be removed in reverse post
>          // order.
> +        discardReferencesTo(*iter, RefType_DefaultNoAssert);

Nit: update the comment (the "resume points are expected to be removed separately" part is no longer true).
Attachment #8462535 - Flags: review?(jdemooij) → review+
Blocks: 1048414
Created attachment 8476665 [details] [diff] [review]
part 3.5 - Discard the resume point if getInlineableGetPropertyCache does not return the MGetPropertyCache.

I have a test case for this issue, the only issue which explain why I still
haven't included the test case is that it is still made of 283 lines. (used
to be ~60k)

This patch discard the resume point even if the MGetPropertyCache is not
interesting for the inlining rule, and that it is not returned there.
Attachment #8476665 - Flags: review?(efaustbmo)
Comment on attachment 8476665 [details] [diff] [review]
part 3.5 - Discard the resume point if getInlineableGetPropertyCache does not return the MGetPropertyCache.

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

Makes sense to me.

::: js/src/jit/IonBuilder.cpp
@@ +4299,5 @@
> +    MGetPropertyCache *operator->() {
> +        return get();
> +    }
> +    operator bool() {
> +        return get();

is this strictly necessary? I know why it's here, and it's cute, but I'm wondering if it's really justified.
Attachment #8476665 - Flags: review?(efaustbmo) → review+
Comment on attachment 8476665 [details] [diff] [review]
part 3.5 - Discard the resume point if getInlineableGetPropertyCache does not return the MGetPropertyCache.

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

::: js/src/jit/IonBuilder.cpp
@@ +4299,5 @@
> +    MGetPropertyCache *operator->() {
> +        return get();
> +    }
> +    operator bool() {
> +        return get();

I guess we can live without it as the only use case is from the condition in IonBuilder::inlineCallsite.
Based on some last pushes to Try, the JQuery error is likely the same one as the one I reproduced with the TypeScript compiler from which I am trying automatically to extract a small test case.

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

The build error in the above was a missing include in the other set of patches.
The build above does not reproduce the mochitest-3 errors, as seen in comment 13.
I'm seeing this assertion failure:

Assertion failure: resumePointsEmpty(), at js/src/jit/MIRGraph.cpp:905

when running run-typescript.js in js/src/octane. It works at edfcd8993e42 and fails at 26dc1dce0e8f.
Depends on: 1057580
Comment on attachment 8461487 [details] [diff] [review]
Part 1 - Make resume point unique to each instruction.

>+    check->setResumePoint(MResumePoint::Copy(alloc(), current->entryResumePoint()));
I crashed here today...

>+MResumePoint *
>+MResumePoint::Copy(TempAllocator &alloc, MResumePoint *src)
>+{
>+    MResumePoint *resume = new(alloc) MResumePoint(src->block(), src->pc(),
>+                                                   src->caller(), src->mode());
>+    // Copy the operands from the original resume point, and not from the
>+    // current block stack.
>+    if (!resume->operands_.init(alloc, src->stackDepth()))
>+        return nullptr;
... presumably due to this failing for some reason.
(In reply to neil@parkwaycc.co.uk from comment #21)
> Comment on attachment 8461487 [details] [diff] [review]
> Part 1 - Make resume point unique to each instruction.
> 
> >+    check->setResumePoint(MResumePoint::Copy(alloc(), current->entryResumePoint()));
> I crashed here today...

Nice catch, would you be interested in making a patch for fixing it?
Depends on: 1131846
You need to log in before you can comment on or make changes to this bug.