[PATCH] D32783: [PartialInlining] Add frequency based cost analysis

[Easwaran Raman via Phabricator via llvm-commits]

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