IonMonkey: Enable backtracking on Inlining failure.

RESOLVED FIXED in Firefox 56

Status

()

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

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 months ago
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.
(Assignee)

Updated

4 months ago
Assignee: nobody → nicolas.b.pierron
Component: JavaScript Engine → JavaScript Engine: JIT
(Assignee)

Updated

4 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 months ago
Created attachment 8878460 [details] [diff] [review]
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.
Attachment #8878460 - Flags: review?(jdemooij)
(Assignee)

Comment 2

4 months ago
Created attachment 8878461 [details] [diff] [review]
IonMonkey: Enable backtracking on inlining failures.

Turn on the flag in a single patch, such that we can revert this change easily.
Attachment #8878461 - Flags: review?(jdemooij)
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/
Attachment #8878460 - Flags: review?(jdemooij) → review+
Comment on attachment 8878461 [details] [diff] [review]
IonMonkey: Enable backtracking on inlining failures.

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

Thanks!
Attachment #8878461 - Flags: review?(jdemooij) → review+
(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

4 months ago
Pushed by npierron@mozilla.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dac1bc10b839
https://hg.mozilla.org/mozilla-central/rev/603e5ded7f5d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

4 months ago
Depends on: 1375435
You need to log in before you can comment on or make changes to this bug.