Bug 226258 – AirAllocateStackByGraphColoring should use the optimized interference graphs from AirAllocateRegistersByGraphColoring

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Morisset]

This would help especially on some Wasm code that has surprisingly dense interference graphs for its stack slots.
In particular, tsf-wasm has a couple of functions for which it is roughly 2MB.
Even worse, mruby-wasm.aotoki.dev (which caused us to lower Options::maximumTmpsForGraphColoring due to jetsams) has two functions for which it is respectively 215MB and 395MB (if we increase Options::maximumTmpsForGraphColoring from 25k to 50k, as they have roughly 30k temporaries).

Early tests indicate that reusing the interference graph datastructures that I added in https://bugs.webkit.org/show_bug.cgi?id=225848 would decrease these sizes:
- for test-wasm, from 2MB to roughly 70kB
- for mruby-wasm.aotoki.dev, from 215MB to 7MB and from 395MB to 11MB
It would also have a large decrease in percentage for most other functions, but with much less impact since most JS functions only have a handful of spill slots if any at all.

[Robin Morisset]

Created attachment 429733 [details]
WIP

[Robin Morisset]

Created attachment 429786 [details]
Patch

[Mark Lam]

Looks like the jsc EWS bot failures are real.  Can you investigate?

[Robin Morisset]

(In reply to Mark Lam from comment #3)
> Looks like the jsc EWS bot failures are real.  Can you investigate?

Looking at it.

[Robin Morisset]

Created attachment 429915 [details]
Patch

The test failures were caused by a debugging option that I had forgotten to reset to false before committing (AirAllocateStackByGraphColoringInternal::reportLargeMemoryUses).

I also tested it on JetStream2 and RAMification, and the patch appears neutral.

[Robin Morisset]

Comment on attachment 429915 [details]
Patch

After discussing with Phil, I appear to have some problem with coalescing which I really should fix before trying to land this.

[Robin Morisset]

Created attachment 429934 [details]
Patch

Fix a stupid infinite loop in remap() !
Also avoided creating self-interfering edges, so we no longer need to be on the lookout for those.

[Filip Pizlo]

Comment on attachment 429934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429934&action=review

> Source/JavaScriptCore/b3/air/AirAllocateRegistersByGraphColoring.cpp:1365
> +        if constexpr (reportInterferenceGraphMemoryUse && (std::is_same<InterferenceSet, WTF::LargeInterferenceGraph>::value || std::is_same<InterferenceSet, WTF::HugeInterferenceGraph>::value)) {

You should do “using WTF::LargeInterferenceGraph” (and ditto for Huge) at the bottom of the file in WTF that defines these things. That way you don’t have to say WTF:: here. That’s what other WTF ADTs do.

[EWS]

/Volumes/Data/worker/Commit-Queue/build/Source/WTF/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Robin Morisset]

Created attachment 429965 [details]
Patch for landing

Thanks Phil for the review.
I added the using statements in wtf/InterferenceGraph.h as suggested.

[Robin Morisset]

Created attachment 429966 [details]
Patch for landing

I had missed a couple of redundant "WTF::"

[EWS]

Committed r278186 (238229@main): <https://commits.webkit.org/238229@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429966 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78599520>