This is an archive of the discontinued LLVM Phabricator instance.

[NFC][regalloc] Introduce the RegAllocEvictionAdvisorAnalysis
ClosedPublic

Authored by mtrofin on Dec 14 2021, 12:06 AM.

Details

Summary

This patch introduces the eviction analysis and the eviction advisor,
the default implementation, and the scaffolding for introducing the
other implementations of the advisor.

Diff Detail

Event Timeline

mtrofin created this revision.Dec 14 2021, 12:06 AM
mtrofin requested review of this revision.Dec 14 2021, 12:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2021, 12:06 AM
mtrofin added inline comments.Dec 14 2021, 12:09 AM
llvm/lib/CodeGen/RegAllocEvictionAdvisor.h
276

to clarify, I plan to do the TODO after figuring out the RegAllocGreedy.h (as probably this would go there instead).

wenlei added inline comments.Dec 15 2021, 4:59 PM
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp
39–43

Can we keep this in greedy?

52–53

Why do we need hierarchy for analysis? I would imagine that the inputs for different advisor are similar, in which case, would it be cleaner if we use one analysis that dispatches to different advisor based on Mode/Kind? That is what InlineAdvisorAnalysis does.

90–91

LLVM_DEBUG

114–116

nit: move this piece below into a helper for use in both advisor and RAGreedy.

EnableLocalReassignment ||
                          MF.getSubtarget().enableRALocalReassignment(
                              MF.getTarget().getOptLevel())

This also reminds me of the advisor layer issue discussed in the previous patch. The advisor is tied to greedy. Would be good to address that soon.

llvm/lib/CodeGen/RegAllocEvictionAdvisor.h
238

nit: Kind->Mode to be consistent with inline advisor?

mtrofin added inline comments.Dec 15 2021, 6:30 PM
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp
39–43

We can, and just pass it through. I can try that.

52–53

In the inliner case, the scope is module-wide, i.e. all the accounting and logging we do is per module. Here, advisors are per function, but we still need to have state shared by the ML advisors: the model evaluators (expensive to set up, cheap to use, and not tied to any particular function); and logs, in the training case, which are per-function, but we want to serialize them all once at doFinalization, to produce one file.

114–116

I could frontend that, but I'm not sure it would help in this case: we don't appear to be using this field anywhere else in RAGreedy, it's only used by the methods that would move with the default advisor. (Maybe I'm missing something though?)

mtrofin marked an inline comment as done.Dec 15 2021, 6:54 PM
mtrofin added inline comments.
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp
90–91

I think we want to warn back to the user. Maybe we should instead mark somehow the created object (which can only be the Default anyway) and in its doInitialization, where we have a LLVMContext, emit an error? (not sure if there's a precedent)

alternatively: we can have the analysis as a final class and delegate to an implementation which we create in doInitialization. The implementation then can have additional requirements for doInitialization/doFinalization and getAnalysisUsage, if it needs.

wdyt?

wenlei added inline comments.Dec 16 2021, 12:18 AM
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp
52–53

Okay, so IIUC, the states that need to be shared by advisors are specific to different Modes? Otherwise, we could still have a generic analysis.

I'm asking because by looking at the default analysis it doesn't seem necessary, but it might make sense if it's needed for the actual ML advisors.

90–91

I think we want to warn back to the user. Maybe we should instead mark somehow the created object (which can only be the Default anyway) and in its doInitialization, where we have a LLVMContext, emit an error? (not sure if there's a precedent)

It feels to me that this is just an intermediate state. The switch uses enum so it has to be one of the three values, then when all modes are implemented, such error path should never happen. If it's just to guard against misuse in this intermediate state, I guess an assertion is fine too as it's temporary anyways?

114–116

Took another look, you're right. sorry for the confusion.

mtrofin marked 3 inline comments as done.Dec 16 2021, 8:32 AM
mtrofin added inline comments.
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp
52–53

Okay, so IIUC, the states that need to be shared by advisors are specific to different Modes?

Yes, that is correct. The Release mode would use an AOT-ed model evaluator. The Development mode uses an interpreted evaluator, and also has the logging infrastructure to manage. In that respect, it's akin to the InlineAdvisor.

Otherwise, we could still have a generic analysis.

I'm asking because by looking at the default analysis it doesn't seem necessary, but it might make sense if it's needed for the actual ML advisors.

Indeed, that's the

90–91

It's helpful for testing the flag works correctly. Also, because we need build opt-in for mlgo, it would be possible to build clang without it but then turn around and ask for 'release' mode, for example. We should message that to the user. I guess we could llvm_unreachable, too - basically, I just don't want to silently go to default.

One advantage of messaging through LLVMContext would be that we could easily turn the error into a warning, and also we avoid ICEing like llvm_unreachable does. I'll prototype something.

114–116

Ack. So we can then leave the flag definition here, too, correct?

wenlei added inline comments.Dec 16 2021, 9:13 AM
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp
114–116

Sounds good.

mtrofin updated this revision to Diff 394924.Dec 16 2021, 10:54 AM
mtrofin marked 4 inline comments as done.

Incorporated naming feedback, also handing unsupported advisor case through regular diagnostics mechanisms (Ctx.error)

mtrofin marked 4 inline comments as done.Dec 16 2021, 10:55 AM
wenlei added inline comments.Dec 16 2021, 4:22 PM
llvm/include/llvm/Analysis/ReplayInlineSettings.h
1 ↗(On Diff #394924)

Is this an intentional change/refactor to include here?

mtrofin updated this revision to Diff 395031.Dec 16 2021, 4:30 PM

removed unintended addition

llvm/include/llvm/Analysis/ReplayInlineSettings.h
1 ↗(On Diff #394924)

no, thanks for the catch. forgot a git add and did a git checkout

wenlei accepted this revision.Dec 16 2021, 4:40 PM

lgtm, thanks.

This revision is now accepted and ready to land.Dec 16 2021, 4:40 PM
This revision was landed with ongoing or failed builds.Dec 16 2021, 5:57 PM
This revision was automatically updated to reflect the committed changes.