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

[Easwaran Raman via Phabricator via llvm-commits]

eraman marked 8 inline comments as done.
eraman added a comment.

Thanks for the detailed comments. In the course of addressing these, I've added an isColdCallSite and isHotCallSite to ProfileSummaryInfo and moved that logic out of inline cost.



================
Comment at: include/llvm/Transforms/Utils/Cloning.h:187
+          nullptr,
+      std::function<BlockFrequencyInfo &(Function &)> *GetBFI = nullptr)
+      : CG(cg), GetAssumptionCache(GetAssumptionCache), GetBFI(GetBFI) {}
----------------
davidxl wrote:
> chandlerc wrote:
> > My brain is probably just being a bit dense, but is there no way to just give an (optional) BFI (or pair of BFIs) directly? This doesn't need to query recursively the way inline cost does?
> This sounds right. IFI probably just needs a pair of BFIs.
Yes, GetBFI is not needed. Now passing caller and callee  BFI pointers.


================
Comment at: lib/Analysis/InlineCost.cpp:675
     Threshold = MinIfValid(Threshold, Params.ColdThreshold);
+  if (UseCallsiteHotness && PSI && CallBB) {
+    auto &CallerBFI = (*GetBFI)(*Caller);
----------------
chandlerc wrote:
> Why would CallBB ever be null?
You're right, it can never be null. Removed the check. 


================
Comment at: lib/Analysis/InlineCost.cpp:681
+            << "     Callsite count =  " << CallsiteCount.getValue()
+            << (PSI->isHotCount(CallsiteCount.getValue()) ? " (HOT)\n" : "\n"));
+      if (PSI->isHotCount(CallsiteCount.getValue()))
----------------
chandlerc wrote:
> You don't print cold here, just hot?
I never needed it when I was debugging the code. I've added a new set of debug messages based on {callsite/callee} {hotness/coldness}


https://reviews.llvm.org/D28331