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

[Sean Silva via llvm-commits]

silvas added a subscriber: silvas.
silvas added a comment.

Can you split the pass out of PGOInstrumentation.cpp?

Also, there does not appear to be enough testing for the amount of functionality introduced.


================
Comment at: include/llvm/Transforms/Instrumentation.h:86
@@ -85,2 +85,3 @@
 createPGOInstrumentationUsePass(StringRef Filename = StringRef(""));
+ModulePass *createPGOIndirectCallPromotionPass(bool InLTO = false);
 
----------------
Please name the parameter after what it does. What does it control?

Also, whatever it does control can probably be done in a separate patch.

================
Comment at: lib/ProfileData/InstrProf.cpp:91
@@ -90,1 +90,3 @@
 
+std::string getPGOFuncNameLTO(const Function &F) {
+  // First check if these is a metadata.
----------------
This function is confusingly named. How about "get name ignoring per-file mangling" or something? A comment can explain why this is valid for LTO.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:302
@@ -248,1 +301,3 @@
     FuncName = getPGOFuncName(F);
+    if (FuncName != Func.getName()) {
+      LLVMContext &C = F.getContext();
----------------
Please clarify why this is needed in a comment (and possibly a separate patch).

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:973
@@ +972,3 @@
+    Count = TotalCount;
+  if ((double)Count / TotalCount * 100 < ICPPercentThreshold)
+    return false;
----------------
No floating point.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1000
@@ +999,3 @@
+  FunctionType *DirectCalleeType = DirectCallee->getFunctionType();
+  unsigned ParaNum = DirectCalleeType->getFunctionNumParams();
+  CallSite CS(Inst);
----------------
ParamNum

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1025
@@ +1024,3 @@
+                                               InstrProfValueData ValueData[],
+                                               uint32_t NumVals,
+                                               uint64_t TotalCount,
----------------
NumVals is way too generic of a name. Please be more specific. If it is just the number of entries in ValueData then use ArrayRef.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1059
@@ +1058,3 @@
+    TotalCount -= Count;
+    ++NumPromotions;
+  }
----------------
NumPromotions is always just equal to WorkList.size().

Actually, I would remove the terminology "worklist" and just say "promotions".

E.g. add `struct CandidatePromotion` with two members TargetFunction and Count. Then have this function just return a `std::vector<CandidatePromotion>`.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1130
@@ +1129,3 @@
+  // Create a phi to unify the return values of calls
+  if (!(Inst->getType()->isVoidTy())) {
+    PHINode *CallRetPhi = PHINode::Create(Inst->getType(), 0);
----------------
Redundant parentheses.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1140
@@ +1139,3 @@
+      BasicBlock::iterator It(NewInst);
+      Instruction *NextInst = &*(++It);
+      NewInst = new BitCastInst(NewInst, CallRetType, "", NextInst);
----------------
std::next (or llvm::next, if we can't use std::next yet).

I.e.

```
if (FuncRetType != CallRetType)
  NewInst = new BitCastInst(NewInst, CallRetType, "", std::next(NewInst));
```

Also, I see you are using IRBuilder above. Any reason why IRBuilder is not preferred for the IR building later in the function?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1184
@@ +1183,3 @@
+// annotation to perform indirect-call promotion.
+bool ICallPromotionFunc::performPromotion() {
+  PGOIndirectCallSiteVisitor ICV;
----------------
The naming here is quite poor. We have 3 functions:

performPromotion
doPromotion
doVersion

I think a better naming is:

processFunction
tryToPromote
promote

respectively.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1188
@@ +1187,3 @@
+  bool Changed = false;
+  for (auto &I : ICV.IndirectCallInsts) {
+    uint32_t NumVals;
----------------
In a separate patch, please add a helper function `findIndirectCallSites(F)` so that you can do `for auto &I : findIndirectCallSites(F))` instead of having this boilerplate manually calling visit.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1198
@@ +1197,3 @@
+    uint32_t NumPromoted = 0;
+    if (analyzeCandidates(I, ValueDataArray.get(), NumVals, TotalCount,
+                          WorkList) > 0)
----------------
getCandidatePromotionsForCallSite is a better name.

```
auto CandidatePromotions = getCandidatePromotionsForCallSite(...);
NumPromoted += tryToPromote(ICall, CandidatePromotions, TotalCount)
```

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1213
@@ +1212,3 @@
+    annotateValueSite(*M, *I, &(ValueDataArray.get()[NumPromoted]),
+                      NumVals - NumPromoted, TotalCount,
+                      IPVK_IndirectCallTarget, MaxNumPromotions);
----------------
If you had ValueDataArray in an ArrayRef (and annotateValueSite took an ArrayRef), then this would be `annotateValueSite(*M, *I, ValueDataArray.drop_front(NumPromoted, TotalCount, IPVK_IndirectCallTarget, MaxNumPromotions)` which would be a lot nicer.

================
Comment at: test/Transforms/PGOProfile/indirect_call_annotation.ll:2
@@ -1,3 +1,3 @@
 ; RUN: llvm-profdata merge %S/Inputs/indirect_call.proftext -o %t.profdata
-; RUN: opt < %s -pgo-instr-use -pgo-test-profile-file=%t.profdata -S | FileCheck %s --check-prefix=VP-ANNOTATION
+; RUN: opt < %s -pgo-instr-use -icp-max-prom=3 -pgo-test-profile-file=%t.profdata -S | FileCheck %s --check-prefix=VP-ANNOTATION
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
Do not piggy-back on existing tests. You are adding completely new functionality which should have its own independent tests.


http://reviews.llvm.org/D17864