[PATCH] D22182: Refactor indirect call promotion profitability analysis (NFC)

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: include/llvm/Analysis/IndirectCallPromotionAnalysis.h:60
@@ +59,3 @@
+  ArrayRef<InstrProfValueData>
+  getPromotionCandidatesForInstruction(const Instruction *I, uint32_t &NumVals,
+                                       uint64_t &TotalCount,
----------------
tejohnson wrote:
> xur wrote:
> > davidxl wrote:
> > > This analysis does not check legality, why is that?
> > I guess this can be a super set of the real transformations. this just to add edges.
> Because the legality check is based on the target function, and this is being refactored so that it can be called when the target function is not yet known (when building the summary in the ThinLTO compile step where it is an inter-module indirect call).
> I guess this can be a super set of the real transformations. this just to add edges.

Right

================
Comment at: lib/Analysis/IndirectCallPromotionAnalysis.cpp:69
@@ +68,3 @@
+                                   cl::desc("Run indirect-call promotion for "
+                                            "invoke instruction only"));
+
----------------
xur wrote:
> icp-call-only and -icp-invoke-only are mainly for debug. I think it's better to keep together with the transformation code.
Why not have them here and prevent adding the ThinLTO edges in the debug case?

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:149
@@ -190,3 +148,3 @@
       Instruction *Inst, const ArrayRef<InstrProfValueData> &ValueDataRef,
-      uint64_t TotalCount);
+      uint64_t TotalCount, uint32_t NumCandidates);
 
----------------
davidxl wrote:
> Document the parameters including the new one.
Ok

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:595
@@ -661,1 +594,3 @@
+    annotateValueSite(*M, *I, ICallProfDataRef.slice(NumPromoted), TotalCount,
+                      IPVK_IndirectCallTarget, NumPromoted);
   }
----------------
xur wrote:
> is the last argument correct? I think it should be NumCandidates.
Yes I think you are right, let me fix that.


http://reviews.llvm.org/D22182