[PATCH] D32783: [PartialInlining] Add frequency based cost analysis
Easwaran Raman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 4 17:46:29 PDT 2017
eraman added inline comments.
================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:107
NumPartialInlining >= MaxNumPartialInlining);
}
+ CallSite getCallSite(User *U) {
----------------
Add an empty line between functions (her and in other places)
================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:388
+
+ BranchProbability RelativeFreq = BranchProbability::getBranchProbability(
+ OutliningCallFreq.getFrequency(), CalleeEntryFreq.getFrequency());
----------------
It wasn't obvious to me why callee entry freq >= outlining call's frequency. I think I understand it now - we ensure the non-return block has a single predecessor and hence can't be in a loop, but a comment above would help.
================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:435
+ if (SwitchInst *SI = dyn_cast<SwitchInst>(I)) {
+ InlineCost += (SI->getNumCases() + 1);
+ continue;
----------------
This is not in multiples of InstrCost (and also inliner's switch cost computation is more sophisticated now, but you've the TODO above)
================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:467
+ }
+
+ int OutliningRuntimeOverhead =
----------------
may be add an assert that OutlinedFunctionCost >= OutlinedRegionCost to potentially catch any bugs.
================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:589
// Extract the body of the if.
+ BasicBlock *codeReplacer = 0;
Function *ExtractedFunction =
----------------
nullptr instead of 0
================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:23
#include "llvm/Analysis/BranchProbabilityInfo.h"
+#include "llvm/Analysis/InlineCost.h"
#include "llvm/Analysis/LoopInfo.h"
----------------
Why do you need this?
================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:754
header);
+ if (codeReplacerP)
+ *codeReplacerP = codeReplacer;
----------------
Why not grab the (only)user to this function in unswitchFunction and get the basic block from there. It adds some overhead, but IMO is cleaner than this.
https://reviews.llvm.org/D32783
More information about the llvm-commits
mailing list