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

[David Li via llvm-commits]

davidxl added a comment.

Thanks for collecting the data!

I have a couple of more comments below:


================
Comment at: lib/Analysis/InlineCost.cpp:618
@@ -598,1 +617,3 @@
   }
+  Optional<uint64_t> CallSiteCount =
+      llvm::getBlockCount(CS.getInstruction()->getParent(), BFA);
----------------
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.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:427
@@ +426,3 @@
+void Inliner::copyBlockFrequency(BasicBlock *Src, BasicBlock *Dst) {
+  Function *F = Src->getParent();
+  BlockFrequencyInfo *BFI = BFA->getBlockFrequencyInfo(F);
----------------
Needs to be guarded with EnableProfile?

================
Comment at: lib/Transforms/IPO/Inliner.cpp:441
@@ -359,2 +440,3 @@
   CallGraph &CG = getAnalysis<CallGraphWrapperPass>().getCallGraph();
+  HasProfileData = hasProfileData(CG.getModule());
   ACT = &getAnalysis<AssumptionCacheTracker>();
----------------
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.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:582
@@ +581,3 @@
+        // block containing the call.
+        copyBlockFrequency(CallSiteBlock, CallSuccessor->getParent());
+
----------------
Should this be guarded with EnableProfile too?


http://reviews.llvm.org/D16381