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

[Rong Xu via llvm-commits]

xur marked 10 inline comments as done.
xur added a comment.

Sean: Thanks for the detailed suggestions. I'll split the new feature into a separated file.

I will also put some of changes into a separated patch.

I do have some other tests locally -- I just not yet to convert them into the unit tests. I'll add them alone with the review goes.


================
Comment at: lib/ProfileData/InstrProf.cpp:91
@@ -90,1 +90,3 @@
 
+std::string getPGOFuncNameLTO(const Function &F) {
+  // First check if these is a metadata.
----------------
silvas wrote:
> 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.
This is to deal with the interaction with LTO's internalization. I'll split this part of change into a separated patch.

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

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1025
@@ +1024,3 @@
+                                               InstrProfValueData ValueData[],
+                                               uint32_t NumVals,
+                                               uint64_t TotalCount,
----------------
silvas wrote:
> 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.
will use a ArrayRef

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1140
@@ +1139,3 @@
+      BasicBlock::iterator It(NewInst);
+      Instruction *NextInst = &*(++It);
+      NewInst = new BitCastInst(NewInst, CallRetType, "", NextInst);
----------------
silvas wrote:
> 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?
No particular reason that I don't use IRbuilder. The inserted instructions are not in different BBs (not with the original call inst). I just thought it's convenient to change the code stream directly.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1188
@@ +1187,3 @@
+  bool Changed = false;
+  for (auto &I : ICV.IndirectCallInsts) {
+    uint32_t NumVals;
----------------
silvas wrote:
> 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.
ok. will do a separated patch.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1213
@@ +1212,3 @@
+    annotateValueSite(*M, *I, &(ValueDataArray.get()[NumPromoted]),
+                      NumVals - NumPromoted, TotalCount,
+                      IPVK_IndirectCallTarget, MaxNumPromotions);
----------------
silvas wrote:
> 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.
yes. this is cleaner. but this requires a annotateValueSite() change. I prefer to do this in a post-commit cleanup patch.

================
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"
----------------
silvas wrote:
> Do not piggy-back on existing tests. You are adding completely new functionality which should have its own independent tests.
undo the change.


http://reviews.llvm.org/D17864