Closed Bug 1662366 Opened 4 years ago Closed 4 years ago

Warp: fix code TODOs

Categories

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

task

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Now that we're more feature complete, we should start addressing the remaining TODO comments.

The ones in maybeInlineIC have been addressed. The one about extra var environments
isn't really a TODO for Warp specifically.

Also change the stub data copying to just use nullptr if there's no stub data.
This is fairly common for simpler IC stubs.

Allowlisting this op in the checker is wrong because we don't want a
MagicValue to show up for JSOp::ToString in Baseline.

Depends on D88960

Use TODO(post-Warp) for TODOs that can only be addressed when IonBuilder is gone.

Remove some TODOs that are no longer relevant or don't need to be addressed
short-term.

Depends on D88961

Depends on D88962

Longer-term more CompileInfo fields should be moved into WarpSnapshot, we can
then dump these fields.

Depends on D88963

Initially the plan was to reuse TraceCacheIRStub but this patch adds the tracing
code for Warp separately because:

  • CacheIR stub data in Warp contains nursery indexes. It's nice to keep that out of the IC-tracing code.
  • We can use WarpGCPtr similar to other snapshotted GC pointers. It asserts GC things are not moved.

Depends on D88964

This adds an OOL path to handle null/undefined => globalThis without a VM call.
IonBuilder optimizes that case based on TI and this ensures we're not a lot slower
for that.

Also give the MIR instruction an empty AliasSet so we can optimize it better in
inlined functions. We no longer have the thisValue hook mentioned in the comment.

Keywords: leave-open
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/895c8b18a46b part 1 - Remove some TODOs in WarpOracle. r=iain https://hg.mozilla.org/integration/autoland/rev/26c1c9e63455 part 2 - Call setImplicitlyUsedUnchecked for JSOp::ToString. r=iain https://hg.mozilla.org/integration/autoland/rev/79068e7fabea part 3 - Remove/rename some TODOs in WarpBuilder. r=iain https://hg.mozilla.org/integration/autoland/rev/3b77c688e440 part 4 - Rename some TODOs to TODO(post-Warp). r=iain https://hg.mozilla.org/integration/autoland/rev/4d27ace84a3e part 5 - Print CompileInfo pointer in WarpCacheIR::dumpData. r=iain https://hg.mozilla.org/integration/autoland/rev/4259f11d48a1 part 6 - Trace cloned CacheIR stub data. r=iain https://hg.mozilla.org/integration/autoland/rev/237e445d595e part 7 - Some MBoxNonStrictThis optimizations. r=anba
No longer regressions: 1668197

Does this still need to be open?

Flags: needinfo?(jdemooij)
Regressions: 1667336

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)

Does this still need to be open?

There's more to do in a few weeks after we remove IonBuilder/TI, but we can do that in other bugs to simplify release tracking.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(jdemooij)
Resolution: --- → FIXED
Regressions: 1696897
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: