1373323 - IonMonkey: Enable backtracking on Inlining failure.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Nicolas B. Pierron [:nbp]]

Bug 1303399 added a feature to revert the compilation graph in case of inlining failure, unfortunately this caused a few issues such as Bug 1314172.  Last time I checked we also had additional failures in our test suites.

This bug is about turning this feature on again.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Do not move blocks used as anchors for backtracking.

Before this modification, the block created by inlineScriptedCall would be
move, and moving this block would change the block->id().  The assertion
inside the MIRGraph::BackupPoint::restore() caught that.

By checking for the outer-resume-point of the predecessor we prevent this
optimization to move the block which does not belong to the same function,
and thus make it safe to revert the changes, by trashing any new blocks and
instructions.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Enable backtracking on inlining failures.

Turn on the flag in a single patch, such that we can revert this change easily.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8878460 [details] [diff] [review]
IonMonkey: Do not move blocks used as anchors for backtracking.

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

Good find.

::: js/src/jit/IonBuilder.cpp
@@ +1496,5 @@
>          mblock->setHitCount(script()->getHitCount(mblock->pc()));
>  
>      // Optimization to move a predecessor that only has this block as successor
> +    // just before this block.  Skip this optimization if the previous block is
> +    // not part of the same function, as we might have to backtrack on inlining

This comment doesn't match the code? If I understand correctly we're not checking for "same function" but "any inlined function".

Maybe s/not part of the same function/part of an inlined function/

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8878461 [details] [diff] [review]
IonMonkey: Enable backtracking on inlining failures.

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

Thanks!

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> This comment doesn't match the code? If I understand correctly we're not
> checking for "same function" but "any inlined function".
> 
> Maybe s/not part of the same function/part of an inlined function/

Oh, only the first block has an outer resume point? Nevermind my comment then.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dac1bc10b839
IonMonkey: Do not move blocks used as anchors for backtracking. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/603e5ded7f5d
IonMonkey: Enable backtracking on inlining failures. r=jandem

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dac1bc10b839
https://hg.mozilla.org/mozilla-central/rev/603e5ded7f5d