1601599 - Remove loop source notes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jan de Mooij [:jandem]]

After bug 1598548 loops won't have any source note offsets anymore, but there will still be a note indicating the loop kind. This is only used to determine stackPhiCount in Ionbuilder - we should try to remove the stackPhiCount logic or else store it as loop head operand (just like the loop depth and canOsr flag).

Comment 1

[Jan de Mooij [:jandem]]

I think we can remove stackPhiCount, the canIonOSR flag from JSOP_LOOPHEAD and the JSOP_LOOPHEAD source notes.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1601599 part 1 - Fix assert in IonBuilder::setStaticName to account for loop phis. r?tcampbell!

This is necessary once we support OSR for spread expressions.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1601599 part 2 - Add a test for double arrays with loop phis. r?tcampbell!

This test failed with the upcoming changes without bug 1602720, because
the old isNewArray() condition ignored loop phis.

Depends on D56698

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1601599 part 3 - Simplify loop phi code in IonBuilder. r?tcampbell!

Before this bug:

• We did not add phis for expression stack slots that aren't part of the loop.
• We disallowed OSR into Ion at loops that have such non-loop stack slots.

This goes back to bug 980263 to fix the following problem: JSOP_INITPROP in
IonBuilder assumed it found MNewObject on the stack and got confused when it
found a loop phi instead. There was a similar issue with MNewArray.

Since then the affected ops have been fixed and supporting OSR at these loops
(and inserting phis for all stack slots) simplifies the code and fixes a potential
perf cliff.

Depends on D56699

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1601599 part 4 - Support Ion OSR at all loops. r?tcampbell!

Depends on D56700

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1601599 part 5 - Remove JSOP_LOOPHEAD source notes. r?tcampbell!

IonBuilder no longer has to know the loop type.

Depends on D56701

Comment 7

[Jan de Mooij [:jandem]]

Getting this green is turning out to be a bit of a rabbit hole because it required cleaning up some confusing parts of TI, see bug 1603373 and bug 1603708. That was long overdue and I'm happy with how it's all turning out so far.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/307481f32d44
part 1 - Fix assert in IonBuilder::setStaticName to account for loop phis. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/4903b1b93526
part 2 - Add a test for double arrays with loop phis. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/93842c3971f8
part 3 - Simplify loop phi code in IonBuilder. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/d9c1e44eb0ad
part 4 - Support Ion OSR at all loops. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/ba6bf1f3c391
part 5 - Remove JSOP_LOOPHEAD source notes. r=tcampbell

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/307481f32d44
https://hg.mozilla.org/mozilla-central/rev/4903b1b93526
https://hg.mozilla.org/mozilla-central/rev/93842c3971f8
https://hg.mozilla.org/mozilla-central/rev/d9c1e44eb0ad
https://hg.mozilla.org/mozilla-central/rev/ba6bf1f3c391