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

[Easwaran Raman via Phabricator via llvm-commits]

eraman added a comment.

In terms of correctness and heuristics this looks good. I'v a few comments that are mostly stylistic and documentation issues.



================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:49
+                                      cl::init(false), cl::ZeroOrMore,
+                                      cl::Hidden,
+                                      cl::desc("Skip Cost Analysis"));
----------------
make this cl::ReallyHidden since it is only used in testing


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:66
+                             cl::Hidden, cl::ZeroOrMore,
+                             cl::desc("Relative frequency of outline region to "
+                                      "the entry block"));
----------------
The description string and comment does not make it clear whether this is the upper limit (I think it is) or not.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:127
+  // the OutlinedFunction into its callers. Return true
+  // if there is any successfully inlining.
+  bool tryPartialInline(Function *DuplicateFunction,
----------------
typo: successfully -> successful


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:141
             NumPartialInlining >= MaxNumPartialInlining);
   }
+  CallSite getCallSite(User *U) {
----------------
an empty line below


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:175
+  std::tuple<int, int, int>
+  computeOutliningCosts(Function *F, const FunctionOutliningInfo *OutliningInfo,
+                        Function *OutlinedFunction,
----------------
Not sure if this has been clang-formatted because it looks like the next two lines can be merged into one (I may be wrong)


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:178
+                        BasicBlock *OutliningCallBB);
+  // Compute the 'Inline' of block BB. InlineCost is a proxy usedto approximate
+  // both the size and runtime cost (Note that in the current inline cost
----------------
Inline -> InlineCost. usedto->used to


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:161
+    CallSite CS = getOneCallSiteTo(F);
+
+    DebugLoc DLoc = CS.getInstruction()->getDebugLoc();
----------------
Empty line?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:397
+
+  BlockFrequency EntryFreq =
+      BFI->getBlockFreq(&DuplicateFunction->getEntryBlock());
----------------
I prefer auto here and below since the get methods clearly convey the type.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:406
+  // When profile data is not available, we need to be very
+  // conservative in estimating the overall savings:
+  if (!hasProfileData(F, OI)) {
----------------
This certainly need some more comments. Essentially you don't want to over-estimate the savings and hence you assume that outlinined region is at least OutlineRegionFreqPercent likely even if BFI tells otherwise.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:407
+  // conservative in estimating the overall savings:
+  if (!hasProfileData(F, OI)) {
+    if (OutlineRegionRelFreq < BranchProbability(OutlineRegionFreqPercent, 100))
----------------
Flip the branch condition and so an early return?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:408
+  if (!hasProfileData(F, OI)) {
+    if (OutlineRegionRelFreq < BranchProbability(OutlineRegionFreqPercent, 100))
+      OutlineRegionRelFreq = BranchProbability(OutlineRegionFreqPercent, 100);
----------------
std::min


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:458
+
+  BranchProbability RelativeFreq =
+      getOutliningCallBBRelativeFreq(F, OI, Callee, CalleeBFI, OutliningCallBB);
----------------
auto here and in the next line.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:564
+      // For the old pass manager:
+      if (!GetBFI) {
+        if (CurrentCallerBFI)
----------------
move the BFI computation to a lambda maybe?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:761
+  auto CalleeEntryCount = F->getEntryCount();
+  uint64_t CalleeEntryC = (CalleeEntryCount ? *CalleeEntryCount : 0);
+  bool AnyInline = false;
----------------
Instead of using two variables with similar names, why not initialize the uint64_t to 0 above the previous if and inside the if do F->getEntryCount()->getValue() ? 


https://reviews.llvm.org/D32783