[PATCH] D28331: Improve PGO support for the new inliner

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

This is looking pretty close. Couple of nits and a couple of more interesting comments inline.



================
Comment at: lib/Analysis/InlineCost.cpp:658
+      if (PSI->isHotCallSite(CS, CallerBFI)) {
+        DEBUG(llvm::dbgs() << "Hot callsite.\n");
         Threshold = MaxIfValid(Threshold, Params.HotCallSiteThreshold);
----------------
nit: Here and below you shouldn't need the `llvm::` qualifier on `dbgs` I think.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:769-773
+  auto GetBFILambda = [&](Function &F) -> BlockFrequencyInfo & {
+    return FAM.getResult<BlockFrequencyAnalysis>(F);
+  };
 
+  function_ref<BlockFrequencyInfo &(Function &)> GetBFI(GetBFILambda);
----------------
I think you can name the lambda `GetBFI` and below pass it as `{GetBFI}` if `GetBFI` doesn't work. You shouldn't need to construct a variable for the function_ref.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1396
   }
 }
+/// Update the block frequencies of the caller after a callee has been inlined.
----------------
missing vertical whitespace here.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1409
+  SmallPtrSet<BasicBlock *, 16> ClonedBBs;
+  for (auto const &Entry : VMap) {
+    if (!isa<BasicBlock>(Entry.first) || !Entry.second)
----------------
The value map seems much larger than the set of basic blocks cloned? Why not walk the cloned region of basic blocks directly and just look them up in the value map?


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:2241-2243
   if (InlinedMustTailCalls && pred_begin(AfterCallBB) == pred_end(AfterCallBB))
     AfterCallBB->eraseFromParent();
+  else if (IFI.CallerBFI) {
----------------
If the else needs braces, use them on the if?


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:2244-2246
+    // Copy original BB's block frequency to AfterCallBB
+    IFI.CallerBFI->setBlockFreq(
+        AfterCallBB, IFI.CallerBFI->getBlockFreq(OrigBB).getFrequency());
----------------
The location of this makes it easy to completely miss...

I wonder, would it be better to just do this immediately when we create AfterCallBB even though we'll delete it in some cases?


https://reviews.llvm.org/D28331