1042729 - Clean the MBasicBlock list of resume point.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

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.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: 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.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: 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.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: 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.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: 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.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: 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.

Comment 6

[Eric Faust [:efaust]]

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.

Comment 7

[Hannes Verschore [:h4writer]]

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()"?

Comment 8

[Nicolas B. Pierron [:nbp]]

(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 9

[Hannes Verschore [:h4writer]]

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.

Comment 10

[Nicolas B. Pierron [:nbp]]

(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.

Comment 11

[Jan de Mooij [:jandem]]

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).

Comment 12

[Nicolas B. Pierron [:nbp]]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6196ec73ac9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ecf21c3c4a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/749e28d0377e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f56127783b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d35b006a2f6

Comment 13

[Ed Morley [:emorley]]

Backed out for mochitest assertions:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2d35b006a2f6
https://tbpl.mozilla.org/php/getParsedLog.php?id=45242154&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45242108&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/71a55df665e4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7e198397b8a1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0b277b8255
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/208cf26c5af8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f9247db062c

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: 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.

Comment 15

[Eric Faust [:efaust]]

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.

Comment 16

[Nicolas B. Pierron [:nbp]]

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.

Comment 17

[Nicolas B. Pierron [:nbp]]

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.

Comment 18

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a48a86bf2ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9a6d2b3f67
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8802bd12fb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5f711e25f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/26dc1dce0e8f

I've folded part 3.5 into part 3, as it modifies most of part 3.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a48a86bf2ef
https://hg.mozilla.org/mozilla-central/rev/fa9a6d2b3f67
https://hg.mozilla.org/mozilla-central/rev/c8802bd12fb2
https://hg.mozilla.org/mozilla-central/rev/9e5f711e25f7
https://hg.mozilla.org/mozilla-central/rev/26dc1dce0e8f

Comment 20

[Dan Gohman [:sunfish]]

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.

Comment 21

[[e-mail]]

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.

Comment 22

[Nicolas B. Pierron [:nbp]]

(In reply to [e-mail] 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?