Closed Bug 1601599 Opened 1 year ago Closed 1 year ago

Remove loop source notes

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox73 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(5 files)

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).

Priority: -- → P3

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

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Depends on: 1602720

This is necessary once we support OSR for spread expressions.

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

Depends on D56698

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

IonBuilder no longer has to know the loop type.

Depends on D56701

Blocks: 1601072
Depends on: 1603373

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.

Pushed by jdemooij@mozilla.com:
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
Blocks: 1604105
You need to log in before you can comment on or make changes to this bug.