Closed Bug 1567438 Opened 3 months ago Closed 3 months ago

Clean up Ion bailouts

Categories

(Core :: JavaScript Engine: JIT, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(14 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
86.54 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review

We can now resume in the interpreter and this simplifies a number of things, for example we no longer have to worry about unsynced R0/R1 stack values.

However there's more to clean up, for example the enter-monitor-chain code can be removed if we monitor this Value in C++.

When we resumeAfter a JOF_TYPESET op, we had code to enter the type monitor
chain for the bailing op, passing it the top stack value. It's simpler to do
this monitoring in C++ in FinishFromBailout and then treat it as a regular
resumeAfter bailout.

The code for this will be simplified more later in the stack.

We now always set the frame's environment chain to a non-null value and have
an explicit isPrologueBailout flag instead of relying on envChain == nullptr.

Depends on D38860

The old code happened to work but didn't really make sense and I ran into some assertion
failures with other changes. Later patches will clean this up more.

Depends on D38861

The old code happened to work for Baseline JIT bailouts because the JIT ignores
the return value slot for noScriptRval scripts, but when bailing out to the
interpreter we would have an optimized-out MagicValue in the return value slot
and get confused.

Depends on D38862

We need this for the next patch because we have a JSOp instead of a pc when
generating the interpreter. (In general it's nicer for functions to take a JSOp
instead of a PC because it can be used in strictly more contexts.)

Depends on D38863

We need this for interpreter bailouts for bailouts involving inlining.

Depends on D38864

This adds/changes some methods we need later on.

Depends on D38865

There will be more clean up in later patches.

Depends on D38866

Use proper C++ constructors/destructors, use UniquePtr instead of js_free.

Depends on D38867

Making variables in InitFromBailout immutable makes the code a lot easier to reason about.

Depends on D38869

Attachment #9079711 - Attachment description: Bug 1567438 part 1 - Replace the enter-monitor-chain bailout code with type monitoring in C++. r?tcampbell! → Bug 1567438 part 1 - Replace the enter-monitor-chain bailout code with type monitoring in C++. r?tcampbell!,iain!
Attachment #9079711 - Attachment description: Bug 1567438 part 1 - Replace the enter-monitor-chain bailout code with type monitoring in C++. r?tcampbell!,iain! → Bug 1567438 part 1 - Replace the enter-monitor-chain bailout code with type monitoring in C++. r?tcampbell,iain
Attached patch Combined patch (obsolete) — Splinter Review
Blocks: 1567920
Attachment #9079730 - Attachment is obsolete: true
Attachment #9079711 - Attachment description: Bug 1567438 part 1 - Replace the enter-monitor-chain bailout code with type monitoring in C++. r?tcampbell,iain → Bug 1567438 part 1 - Replace the enter-monitor-chain bailout code with type monitoring in C++. r?tcampbell!,iain!

Instead of storing both buffer_ and header_ we now store just header_ (as UniquePtr).

This also removes HeaderSize() and uses sizeof(BaselineBailoutInfo) directly.

Depends on D38975

Keywords: leave-open
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58ea17fc315e
part 1 - Replace the enter-monitor-chain bailout code with type monitoring in C++. r=tcampbell,iain
https://hg.mozilla.org/integration/autoland/rev/ff457522f563
part 2 - Clean up environment chain and prologue bailout handling. r=tcampbell,iain
https://hg.mozilla.org/integration/autoland/rev/ea48a9066430
part 3 - Don't skip JSOP_LOOPENTRY ops for prologue bailouts. r=tcampbell,iain
https://hg.mozilla.org/integration/autoland/rev/92cda183a416
part 4 - Don't set the frame's return value for noScriptRval scripts. r=tcampbell,iain
https://hg.mozilla.org/integration/autoland/rev/9267308e0b89
part 5 - Change IsIonInlinablePC to IsIonInlinableOp. r=tcampbell,iain
https://hg.mozilla.org/integration/autoland/rev/ffe515491b2a
part 6 - Save IC return offsets in interpreter code for IsIonInlinableOp ops. r=tcampbell,iain
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92c20b931f96
part 7 - Some BaselineFrame changes. r=tcampbell CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/3c3b12e8c597
part 8 - Change Ion bailouts to resume in the interpreter instead of JIT. r=tcampbell CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/9a1eeba0d050
part 9 - Modernize BaselineBailoutInfo. r=tcampbell CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/cc928df24bc5
part 10 - Remove now-unnecessary AutoSuppressGC in FinishBailoutToBaseline. r=tcampbell CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/6e65f403e1ce
part 11 - Add GetResumePC, make pc and op variables in InitFromBailout const. r=tcampbell CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/098b10e61991
part 12 - Simplify type monitor code a bit and fix a now-stale comment. r=tcampbell,iain CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/e7d2e7780cbd
part 13 - Modernize and simplify BaselineStackBuilder. r=tcampbell CLOSED TREE
Keywords: leave-open
Regressions: 1570926
Regressions: 1572051
You need to log in before you can comment on or make changes to this bug.