[PATCH] D16381: Infrastructure to allow use of PGO in inliner

[Easwaran Raman via llvm-commits]

eraman marked an inline comment as done.

================
Comment at: lib/Analysis/InlineCost.cpp:618
@@ -598,1 +617,3 @@
   }
+  Optional<uint64_t> CallSiteCount =
+      llvm::getBlockCount(CS.getInstruction()->getParent(), BFA);
----------------
davidxl wrote:
> This call has the side effect of force computing BFI even when it is disabled (with option).  I think CallAnalyzer needs to have a wrapper to this function and check the settings before this is called.
I have guarded the computation in getBlockCount by a check to see if BFA is nullptr. Also made sure getInlineCost gets a non-null BFA only when HasProfileData is true.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:441
@@ -359,2 +440,3 @@
   CallGraph &CG = getAnalysis<CallGraphWrapperPass>().getCallGraph();
+  HasProfileData = hasProfileData(CG.getModule());
   ACT = &getAnalysis<AssumptionCacheTracker>();
----------------
davidxl wrote:
> I think we should introduce a more general flag here EnableProfile. This flag's value is determined by an internal option:
> 
> --enable-profile-in-inliner=default|yes|no
> 
> With yes and no can be used to force turning on|off while default is subject to compiler: for now if  hasProfile is true, turn it on otherwise off.
I don't prefer to add a new flag for that. I think the current solution of checking if profile is turned on is sufficient for now.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:582
@@ +581,3 @@
+        // block containing the call.
+        copyBlockFrequency(CallSiteBlock, CallSuccessor->getParent());
+
----------------
davidxl wrote:
> Should this be guarded with EnableProfile too?
The callee copyBlockFrequency has the guard, so it is not necessary at the callsite.


http://reviews.llvm.org/D16381