IonMonkey: Enable backtracking on Inlining failure.

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years 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

2 years 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

2 years ago
Assignee: nobody → nicolas.b.pierron
Component: JavaScript Engine → JavaScript Engine: JIT
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
Assignee

Comment 1

2 years ago
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

2 years ago
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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dac1bc10b839
https://hg.mozilla.org/mozilla-central/rev/603e5ded7f5d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee

Updated

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