[PATCH] D21932: [ThinLTO] Perform profile-guided indirect call promotion

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:265
@@ -263,1 +264,3 @@
 
+  /// Record a call graph edge from this function to each function GUID recorded
+  /// in \p CallGraphEdges.
----------------
davidxl wrote:
> This is a duplicate.
No, this is new (the other one takes a map of Value* this takes a map of GUID

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:99
@@ +98,3 @@
+          const CallInst *CI = dyn_cast<CallInst>(&I);
+          if (CS.getCalledValue() && !isa<Constant>(CS.getCalledValue()) &&
+              !(CI && CI->isInlineAsm())) {
----------------
davidxl wrote:
> When getCalledValue() returns  constant, should it be covered above (i.e., CalledFunction is not null)?
This was modeled on the checking in PGOIndirectCallSiteVisitor::visitCallSite, which checks if the value is constant after the early return when getCalledFunction() is not-null.

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:104
@@ +103,3 @@
+            auto CandidateProfileData =
+                ICallAnalysis.getPromotionCandidatesForInstruction(
+                    &I, NumVals, TotalCount, NumCandidates);
----------------
davidxl wrote:
> See the comment in the dependent patch -- the icall analysis needs to return set of targets also 'legal'.
As noted there, we can't check legality in the refactored code since we don't have access to the called function yet.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:151
@@ +150,3 @@
+    if (Index)
+      for (const auto &I : *Index)
+        for (auto &S : I.second)
----------------
mehdi_amini wrote:
> davidxl wrote:
> > Add one line comment before each 'for' -- kind of hard to read a loop deep nested without some background.
> Alternatively, using explicit name for iterator variable can help.
Ok, add some comments or use a more explicit name.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:366
@@ +365,3 @@
+  // Indirect call promotion that promotes intra-module targets only.
+  MPM.add(createPGOIndirectCallPromotionLegacyPass());
+
----------------
mehdi_amini wrote:
> Is this totally related to this patch or is it already an existing mistake? (I.e. could be committed separately?)
> 
> Also remove braces for the if block.
It could be committed earlier but would be a no-op without the rest of this patch (unless we happened to import a function that was called directly but also happened to be called indirectly).

Will remove braces.


http://reviews.llvm.org/D21932