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

David Li via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 25 22:13:12 PST 2016

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:


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?


More information about the llvm-commits mailing list