[PATCH] D17864: [PGO] Promote indirect calls to conditional direct calls with value-profile

[David Li via llvm-commits]

davidxl added a comment.

First batch of comments from browsing the patch.


================
Comment at: lib/IR/Instructions.cpp:3016
@@ -3012,1 +3015,3 @@
+bool 
+CastInst::castIsValid(Instruction::CastOps op, Type *SrcTy, Type *DstTy) {
 
----------------
This change can be carved out as a NFC refactoring.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:588
@@ -583,1 +587,3 @@
 
+  // Indirect call promotion. This should promote all the targets that left by
+  // the earlier promotion pass that promotes intra-moduletargets.
----------------
that left -- that are left

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:589
@@ +588,3 @@
+  // Indirect call promotion. This should promote all the targets that left by
+  // the earlier promotion pass that promotes intra-moduletargets.
+  // This two-step promotion is to save the compile time. For LTO, it should
----------------
intra-module targets

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:590
@@ +589,3 @@
+  // the earlier promotion pass that promotes intra-moduletargets.
+  // This two-step promotion is to save the compile time. For LTO, it should
+  // produce the same result as if we only do promotion here.
----------------
can you explain the 'save compile time' part here?

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:74
@@ +73,3 @@
+// Set the cutoff value for the promotion. If the value is other than 0, we
+// stop the transformation once the cutoff value is reached.
+// For debug use only.
----------------
once the total number of promotions equals the cutoff value.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:77
@@ +76,3 @@
+static cl::opt<unsigned>
+    ICPCUTOFF("icp-cutoff", cl::init(0), cl::Hidden, cl::ZeroOrMore,
+              cl::desc("Max number of promotions for this compilaiton"));
----------------
Why all upper case, ICPCutOff looks better.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:80
@@ +79,3 @@
+
+// Skip the callsite up to this number if the value is other than 0.
+// For debug use only.
----------------
--> 

If ICPCSSkip is non zero, the first ICPCSSkip callsites will be skipped.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:92
@@ +91,3 @@
+                                         "mode"));
+// Only transform call instruction. For debug use only.
+static cl::opt<bool>
----------------
If the option is set to true, only call instructions will be considered for transformation -- invoke instructions will be ignored.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:149
@@ +148,3 @@
+
+  // Symtab that maps value to instrumented function name.
+  InstrProfSymtab *Symtab;
----------------
maps indirect call profile values to function names.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:169
@@ +168,3 @@
+
+  // A struct that record the direct target and it's call count.
+  struct CandidatePromotion {
----------------
records

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:170
@@ +169,3 @@
+  // A struct that record the direct target and it's call count.
+  struct CandidatePromotion {
+    Function *TargetFunction;
----------------
Perhaps PromotionCandidate?

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:185
@@ +184,3 @@
+
+  uint32_t tryToPromote(Instruction *Inst,
+                        const std::vector<CandidatePromotion> &Candidates,
----------------
Document this method -- what is passed in and what is returned.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:189
@@ +188,3 @@
+
+  const char *StatusToString(const TargetStatus S) {
+    switch (S) {
----------------
make it static member function

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:221
@@ +220,3 @@
+                                               uint64_t TotalCount) {
+  if (TotalCount < ICPCountThreshold || Count < ICPCountThreshold)
+    return false;
----------------
just
 if (count < ICPCountThreshold)

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:223
@@ +222,3 @@
+    return false;
+  if (TotalCount < Count)
+    Count = TotalCount;
----------------
When can this happen? Should it be corrected in caller?

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:234
@@ +233,3 @@
+  StringRef TargetFuncName = Symtab->getFuncName(Target);
+  if (TargetFuncName.empty())
+    return NotAvailableInModule;
----------------
Is this check needed? NotAvailableInModule is essentially body not available.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:278
@@ +277,3 @@
+    uint64_t TotalCount) {
+  uint32_t NumPromotions = 0;
+  uint32_t NumVals = ValueDataRef.size();
----------------
This local variable is not used.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:362
@@ +361,3 @@
+  *DirectCallBB = ThenTerm->getParent();
+  (*DirectCallBB)->setName("if.true");
+  *IndirectCallBB = ElseTerm->getParent();
----------------
make the name more meaningful such as "if.true.direct_targ"

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:537
@@ +536,3 @@
+//
+bool ICallPromotionFunc::promote(Instruction *Inst, Function *DirectCallee,
+                                 uint64_t Count, uint64_t TotalCount) {
----------------
It always returns true -- change it to void


http://reviews.llvm.org/D17864