This patch introduces the eviction analysis and the eviction advisor,
the default implementation, and the scaffolding for introducing the
other implementations of the advisor.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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). |
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp | ||
---|---|---|
38–42 | Can we keep this in greedy? | |
51–52 | 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. | |
89–90 | LLVM_DEBUG | |
113–115 | 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? |
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp | ||
---|---|---|
38–42 | We can, and just pass it through. I can try that. | |
51–52 | 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. | |
113–115 | 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?) |
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp | ||
---|---|---|
89–90 | 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? |
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp | ||
---|---|---|
51–52 | 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. | |
89–90 |
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? | |
113–115 | Took another look, you're right. sorry for the confusion. |
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp | ||
---|---|---|
51–52 |
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.
Indeed, that's the | |
89–90 | 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. | |
113–115 | Ack. So we can then leave the flag definition here, too, correct? |
llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp | ||
---|---|---|
113–115 | Sounds good. |
Incorporated naming feedback, also handing unsupported advisor case through regular diagnostics mechanisms (Ctx.error)
llvm/include/llvm/Analysis/ReplayInlineSettings.h | ||
---|---|---|
1 ↗ | (On Diff #394924) | Is this an intentional change/refactor to include here? |
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 |
nit: Kind->Mode to be consistent with inline advisor?