1045749 - Eliminate resume point operands which are immediately popped

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

If, after bailing out from Ion execution, baseline immediately pops the top of the stack, that popped value doesn't need to be accurately retained in the resume point used for the bailout.  The attached patch performs this optimization, which fixes the codegen inefficiency in bug 1028580 comment 4.

Comment 1

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8464136 [details] [diff] [review]
patch

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

::: js/src/jit/IonAnalysis.cpp
@@ +406,5 @@
>          if (mir->shouldCancel("Eliminate Dead Resume Point Operands (main loop)"))
>              return false;
>  
> +        // Eliminate resume point operands which are trivially unobservable.
> +        for (MResumePointIterator iter(block->resumePointsBegin());

FYI, I am removing this list of resume points in Bug 1042729, can we do that when we are creating the resume points instead of doing it here?

@@ +416,5 @@
> +            if (iter->mode() == MResumePoint::ResumeAt && *iter->pc() == JSOP_POP) {
> +                size_t top = iter->stackDepth() - 1;
> +
> +                // Using MResumePointIterator can see partially initialized resume points.
> +                if (!iter->hasOperand(top))

No longer, this is what is fixed in Bug 1042729.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: don't use MResumePointIterator

This patch avoids using MResumePointIterator, instead looking for resume points attached to instructions and block entry, and doesn't need the check for orphaned uninitialized resume points.  I think it's cleaner to do this here than at resume point creation since this optimization fits in well with what EliminateDeadResumePointOperands is already doing.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8464245 [details] [diff] [review]
don't use MResumePointIterator

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

Stealing review.
Sounds good to me, nice finding :)

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8464245 [details] [diff] [review]
don't use MResumePointIterator

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

::: js/src/jit/IonAnalysis.cpp
@@ +392,5 @@
> +        return;
> +
> +    size_t top = rp->stackDepth() - 1;
> +
> +    MDefinition *def = rp->getOperand(top);

nit: Assert that this operand is not observable.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: updated

Unfortunately, adding that assert exposed an issue with the patch, where we were replacing non-stack operands to resume points.  The problem is with the resume points for split edges, which do not necessarily have a stack depth that matches their associated pc.  Rather than working around these deformed resume points, this patch just clears them and their uses out.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8465576 [details] [diff] [review]
updated

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

Whoa, this is a big change compared to the assumption we have on the graph.

Hopefully SplitEdge blocks should have no instructions, but we would have to be careful with transformations which might be adding instructions in these blocks.
By chance the lowering would simply SEGV with near null pointer if such case happen.

Comment 7

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d052b9190a4c

Comment 8

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/d052b9190a4c