The goal is to simplify experimentation on the cost model. Today,
CallAnalyzer decides 2 things: legality, and benefit. The refactoring
keeps legality assessment in CallAnalyzer, and factors benefit
evaluation out, as an extension.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
virtuals have implementation; renamed derived class; made OnJumpTable return bool, like OnCaseCluster does.
The SROA handling code should also be abstracted away and let the derived class handling cost accumulation. In particular, the common code pattern like:
if (lookupSROAArgAndCost(I.getPointerOperand(), SROAArg, CostIt)) {
if (I.isSimple()) { accumulateSROACost(CostIt, InlineConstants::InstrCost); return true; } disableSROA(CostIt); }
Can be abtracted with interface CallAnalyzer::handleSROA(..), and the derived class implements this pattern.
Done - that also pulls out the sroa-related costing stuff out, which I've missed originally.
llvm/lib/Analysis/InlineCost.cpp | ||
---|---|---|
164 | Document what to return. | |
171 | Called before the analysis of the callee body (with callsite contexts propagate) starts. It checks callsites specific information. ALso document what to return. | |
173 | Called when the analysis engine decides that SROA can not be done. | |
175 | Called by the analysis when load elimination can not happen. | |
182 | Make a note that 'Call' is a callsite in the callee body -- not to be confused with the callsite being analyzed. | |
197 | onFinalizeSwitch | |
206 | analyzeIndirectCall |
The intention is to make the base CallAnalysis becomes a symbolic execution engine (virtual optimizations) what can be reused. The cost tracking is extracted into the derived class.
llvm/lib/Analysis/InlineCost.cpp | ||
---|---|---|
129–131 | This should go to InlineCostCallAnalyzer as it is not needed for legality check or symbolic evaluation. | |
136 | Please leave an empty line before the doc comment to make this more readable (here and other places below) | |
676 | Let's say you are using a different flavor of InlineCostCallAnalyzer (one which has different implemnetation of the virtual onXYZ methods), you should be instantiating an instance of that class here and call analyze here. | |
1519 | Comment should go to callee (please check all places where you call onXYZ methods if the comment belong in the caller or callee) | |
1545 | Why not move the entire block into a single function? | |
1673–1674 | Comments should go into the callee. Also, it's better to merge onCaseCluster and onFinalizeSwitch into one function and just do onFinalizeSwitch(); (Revisiting this - I think even onJumpTable should be folded into onFinalizeSwitch. From simplification/legality perspective, there is notthing left to do here after checking if the condition folds into a constant) | |
1767 | onCommonInstructionSimplification is more meaningful. | |
1967–1968 | The comments are not in-sync with the code. shouldStop could potentially use other ways to check if the analysis should stop - not just cost exceeds the threshold | |
2038–2039 | Shouldn't this entire block be moved to InlineCostCallAnalyzer as this isn't performing symbolic evaluation or legality check? |
llvm/lib/Analysis/InlineCost.cpp | ||
---|---|---|
676 | Maybe, but I'd prefer addressing that problem when the scenario arises, and avoid the added complexity meanwhile, wdyt? | |
1967–1968 | True, and in this case, the code is kind of self-evident. I moved the comment to the shouldStop implementation. | |
2038–2039 | Possibly, but all that prep work - getting a DominatorTree, LoopInfo, going over loops, making sure we ignore those that aren't executed - would be necessary if we want to account the number of loops somehow. |
llvm/lib/Analysis/InlineCost.cpp | ||
---|---|---|
2038–2039 | This looks like cost computation specific and should probably be moved to the derived class. It can query the base class about deadblocks. |
LGTM
llvm/lib/Analysis/InlineCost.cpp | ||
---|---|---|
457 | Please add a FIXME here saying we should be using the same derived class instance |
llvm/lib/Analysis/InlineCost.cpp | ||
---|---|---|
457 | done, also marked InlineCostCallAnalyzer as final and referenced this fixme there, it'll be more visible should deriving InlineCostCallAnalyzer happen. |
llvm/lib/Analysis/InlineCost.cpp | ||
---|---|---|
2226 | It's unrelated, but intended: clang-format kept indenting the original comments. I looked closer at what was happening, and it looks like the comments characterize the 'case' they used to be above, but clang-format wants to indent them as a case statement body. So I moved them as shown, to avoid future churn with clang-format. |
llvm/lib/Analysis/InlineCost.cpp | ||
---|---|---|
241 | Agreed, I'll send a followup to rename. |
This should go to InlineCostCallAnalyzer as it is not needed for legality check or symbolic evaluation.