Bug 226258 - AirAllocateStackByGraphColoring should use the optimized interference graphs from AirAllocateRegistersByGraphColoring
Summary: AirAllocateStackByGraphColoring should use the optimized interference graphs ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on:
Blocks: 226388
  Show dependency treegraph
 
Reported: 2021-05-25 23:58 PDT by Robin Morisset
Modified: 2021-05-28 11:53 PDT (History)
15 users (show)

See Also:


Attachments
WIP (55.08 KB, patch)
2021-05-25 23:59 PDT, Robin Morisset
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (66.30 KB, patch)
2021-05-26 13:05 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (66.29 KB, patch)
2021-05-27 13:19 PDT, Robin Morisset
rmorisset: commit-queue-
Details | Formatted Diff | Diff
Patch (65.98 KB, patch)
2021-05-27 15:05 PDT, Robin Morisset
fpizlo: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (63.92 KB, patch)
2021-05-27 17:43 PDT, Robin Morisset
rmorisset: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (63.34 KB, patch)
2021-05-27 17:45 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2021-05-25 23:58:33 PDT
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.
Comment 1 Robin Morisset 2021-05-25 23:59:31 PDT
Created attachment 429733 [details]
WIP
Comment 2 Robin Morisset 2021-05-26 13:05:53 PDT
Created attachment 429786 [details]
Patch
Comment 3 Mark Lam 2021-05-26 14:43:38 PDT
Looks like the jsc EWS bot failures are real.  Can you investigate?
Comment 4 Robin Morisset 2021-05-26 17:41:27 PDT
(In reply to Mark Lam from comment #3)
> Looks like the jsc EWS bot failures are real.  Can you investigate?

Looking at it.
Comment 5 Robin Morisset 2021-05-27 13:19:26 PDT
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.
Comment 6 Robin Morisset 2021-05-27 13:46:45 PDT
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.
Comment 7 Robin Morisset 2021-05-27 15:05:29 PDT
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.
Comment 8 Filip Pizlo 2021-05-27 16:41:38 PDT
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.
Comment 9 EWS 2021-05-27 16:42:39 PDT
/Volumes/Data/worker/Commit-Queue/build/Source/WTF/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 10 Robin Morisset 2021-05-27 17:43:40 PDT
Created attachment 429965 [details]
Patch for landing

Thanks Phil for the review.
I added the using statements in wtf/InterferenceGraph.h as suggested.
Comment 11 Robin Morisset 2021-05-27 17:45:48 PDT
Created attachment 429966 [details]
Patch for landing

I had missed a couple of redundant "WTF::"
Comment 12 EWS 2021-05-27 19:08:28 PDT
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].
Comment 13 Radar WebKit Bug Importer 2021-05-27 19:09:17 PDT
<rdar://problem/78599520>