1613594 - Decouple IonBuilder from the Ion backend

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

IonBuilder is used for MIR building but also represents the off-thread compilation job. We should decouple this so IonBuilder is just the MIR builder (stack-allocated) so that we can more easily add a second builder.

There's a similar issue with MIRGenerator: it's IonBuilder's base class (and used for Wasm compilation) and is also used by the backend.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 part 1 - Move abortedPreliminaryGroups Vector from MIRGenerator to IonBuilder. r?iain!

IonBuilder is a better fit for this data. MIRGenerator is used by the backend
and is also used for Wasm compilation and WarpBuilder in the future.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 part 2 - Only set the safeForMinorGC_ flag on the outermost builder. r?iain!

This MIRGenerator flag is not used for inline builders so there's no need to set
it. With later patches in this stack there will be a single MIRGenerator per
compilation so this will become a bit more obvious.

Depends on D62007

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 part 3 - Stop using MIRGenerator as base class for IonBuilder. r?iain!

For now MIRGenerator and IonBuilder still have the same lifetime, but later
parts will change IonBuilder to a stack-allocated class with a much shorter
lifetime than MIRGenerator.

Some MIRGenerator fields that are accessed frequently by IonBuilder are now
cached in IonBuilder to avoid an extra dereference.

Depends on D62008

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 part 4 - Use one MIRGenerator per compilation. r?iain!

We now no longer allocate a new MIRGenerator for each inline builder. This was
pretty confusing because only the outermost one survived and was used by the
backend.

MIRGenerator's CompileInfo field was renamed from info_ to outerInfo_ to make
this a bit clearer.

Depends on D62009

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 part 5 - Add IonCompileTask class and use it instead of IonBuilder for off-thread tasks. r?iain!

For now IonCompileTask just forwards to IonBuilder, but that will change in a
later patch.

Depends on D62010

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 part 6 - Change IonCompileTask to not require an IonBuilder. r?iain!

This will let us shorten the IonBuilder lifetime later.

Depends on D62011

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 part 7 - Stop heap-allocating IonBuilder. r?iain!

The pendingEdges_ field no longer has to use Maybe<> because we now always
call IonBuilder's destructor.

Depends on D62012

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 part 8 - Rename ionBuilder memory reporter to ionCompileTask. r?iain!

Depends on D62014

Comment 9

[Jan de Mooij [:jandem]]

There's some follow-up work left: make stack-allocated IonBuilder scope even shorter and move IonCompileTask out of IonBuilder.h/cpp, but let's get this part done first.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 part 9 - Speed up MIRGenerator::isOptimizationTrackingEnabled(). r?iain!

This way we're also guaranteed to get a consistent value per compilation.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/146518d1ebf2
part 1 - Move abortedPreliminaryGroups Vector from MIRGenerator to IonBuilder. r=iain
https://hg.mozilla.org/integration/autoland/rev/f396891236a0
part 2 - Only set the safeForMinorGC_ flag on the outermost builder. r=iain
https://hg.mozilla.org/integration/autoland/rev/c5029fed3987
part 3 - Stop using MIRGenerator as base class for IonBuilder. r=iain
https://hg.mozilla.org/integration/autoland/rev/854f4d27a3f1
part 4 - Use one MIRGenerator per compilation. r=iain
https://hg.mozilla.org/integration/autoland/rev/7b925b0d13e8
part 5 - Add IonCompileTask class and use it instead of IonBuilder for off-thread tasks. r=iain
https://hg.mozilla.org/integration/autoland/rev/50402c669262
part 6 - Change IonCompileTask to not require an IonBuilder. r=iain
https://hg.mozilla.org/integration/autoland/rev/1bc78d155678
part 7 - Stop heap-allocating IonBuilder. r=iain
https://hg.mozilla.org/integration/autoland/rev/fb651fbf6aaa
part 8 - Rename ionBuilder memory reporter to ionCompileTask. r=iain
https://hg.mozilla.org/integration/autoland/rev/64c6f3ce6465
part 9 - Speed up MIRGenerator::isOptimizationTrackingEnabled(). r=iain

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1613594 follow-up fix for 32-bit x86 builds.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e498dfd4119
follow-up fix for 32-bit x86 builds. on a CLOSED TREE

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/146518d1ebf2
https://hg.mozilla.org/mozilla-central/rev/f396891236a0
https://hg.mozilla.org/mozilla-central/rev/c5029fed3987
https://hg.mozilla.org/mozilla-central/rev/854f4d27a3f1
https://hg.mozilla.org/mozilla-central/rev/7b925b0d13e8
https://hg.mozilla.org/mozilla-central/rev/50402c669262
https://hg.mozilla.org/mozilla-central/rev/1bc78d155678
https://hg.mozilla.org/mozilla-central/rev/fb651fbf6aaa
https://hg.mozilla.org/mozilla-central/rev/64c6f3ce6465
https://hg.mozilla.org/mozilla-central/rev/0e498dfd4119