[PGO] IR level instrumentation of indirect call value profiling

[xur]

Summary

This patch adds the instrumentation for indirect call value profiling. It just finds all the indirect call-sites and generates instrprof_value_profile intrinsic calls. A new opt level option -disable-vp is introduced to disable
the value profiling instrumentation.

This patch has dependence on http://reviews.llvm.org/D16015

[davidxl]

Please update the title to be '... IR level instrumentation of ..'

[davidxl]

Please also add test cases.

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

347 ↗

(On Diff #44383)

To make sure that instrumentation and lowering assign the callsite index in the same way, some refactoring is needed -- defined a Icall visitor class that can be shared across.

351 ↗

(On Diff #44383)

Fix format.

[xur]

revised the patch based on the comments: using visitor class now.

[betulb]

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

318 ↗

(On Diff #45031)

I do not like the variables to have names like "Get....". IndirectCallSiteVisitor would have been more appropriate.

345 ↗

(On Diff #44383)

The return should be on its own line. clang-format.

[davidxl]

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

362 ↗

(On Diff #45031)

use it with -style LLVM

[davidxl]

LGTM with the naming nits fixed.

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

318 ↗

(On Diff #45031)

Maybe
PGOIndirectCallSiteVisitor.

[xur]

Thanks for the comments. I'll fix them.

-Rong

[xur]

Name and format change based on the review comments.
Also add a new test case.

Thanks,

-Rong

[vsk]

Thanks xur, sorry for jumping on this late. Two nits --

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

326 ↗

(On Diff #45286)

Is the !CS needed? If it doesn't feel too cramped, maybe the conditional could be CS.getCalledFunction() || !CS.getCalledValue().

406 ↗

(On Diff #45286)

I don't think you need this for your test. Please use a separate commit for this.

[xur]

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

326 ↗

(On Diff #45286)

You are very right. !CS is not need as this is a call instruction. The condition should have simplified to your version too.

406 ↗

(On Diff #45286)

will do.

[xur]

Revised patch based on vsk's review comments.

Thanks,

-Rong

llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

 // BBInfo contains auxiliary information for each BB. These two classes are used
 // in pass PGOInstrumentationGen. Class PGOUseEdge and UseBBInfo are the derived
 // class of PGOEdge and BBInfo, respectively. They contains extra data structure
 // used in populating profile counters.
 // The MST implementation is in Class CFGMST (CFGMST.h).
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/Transforms/Instrumentation.h"
 #include "CFGMST.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/BlockFrequencyInfo.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/Analysis/CFG.h"
+#include "llvm/IR/CallSite.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstIterator.h"
+#include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Pass.h"
 #include "llvm/ProfileData/InstrProfReader.h"
 #include "llvm/Support/BranchProbability.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/JamCRC.h"
+#include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include <string>
 #include <utility>
 #include <vector>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "pgo-instrumentation"
 
 STATISTIC(NumOfPGOInstrument, "Number of edges instrumented.");
 STATISTIC(NumOfPGOEdge, "Number of edges.");
 STATISTIC(NumOfPGOBB, "Number of basic-blocks.");
 STATISTIC(NumOfPGOSplit, "Number of critical edge splits.");
 STATISTIC(NumOfPGOFunc, "Number of functions having valid profile counts.");
 STATISTIC(NumOfPGOMismatch, "Number of functions having mismatch profile.");
 STATISTIC(NumOfPGOMissing, "Number of functions without profile.");
+STATISTIC(NumOfPGOICall, "Number of indirect call value instrumentation.");
 
 // Command line option to specify the file to read profile from. This is
 // mainly used for testing.
 static cl::opt<std::string>
     PGOTestProfileFile("pgo-test-profile-file", cl::init(""), cl::Hidden,
                        cl::value_desc("filename"),
                        cl::desc("Specify the path of profile data file. This is"
                                 "mainly for test purpose."));
 
+// Command line options to disable value profiling. The default is false:
+// i.e. vaule profiling is enabled by default. This is for debug purpose.
+static cl::opt<bool>
+DisableValueProfiling("disable-vp", cl::init(false),
+                                    cl::Hidden,
+                                    cl::desc("Disable Value Profiling"));
+
 namespace {
 class PGOInstrumentationGen : public ModulePass {
 public:
   static char ID;
 
   PGOInstrumentationGen() : ModulePass(ID) {
     initializePGOInstrumentationGenPass(*PassRegistry::getPassRegistry());
   }
public:
   BasicBlock *getInstrBB(Edge *E);
 
   // Return the auxiliary BB information.
   BBInfo &getBBInfo(const BasicBlock *BB) const { return MST.getBBInfo(BB); }
 
   // Dump edges and BB information.
   void dumpInfo(std::string Str = "") const {
     MST.dumpEdges(dbgs(), Twine("Dump Function ") + FuncName + " Hash: " +
                               Twine(FunctionHash) + "\t" + Str);
   }
 
   FuncPGOInstrumentation(Function &Func, bool CreateGlobalVar = false,
                          BranchProbabilityInfo *BPI = nullptr,
                          BlockFrequencyInfo *BFI = nullptr)
       : F(Func), FunctionHash(0), MST(F, BPI, BFI) {
     FuncName = getPGOFuncName(F);
     computeCFGHash();
BasicBlock *FuncPGOInstrumentation<Edge, BBInfo>::getInstrBB(Edge *E) {
   unsigned SuccNum = GetSuccessorNumber(SrcBB, DestBB);
   BasicBlock *InstrBB = SplitCriticalEdge(TI, SuccNum);
   assert(InstrBB && "Critical edge is not split");
 
   E->Removed = true;
   return InstrBB;
 }
 
-// Visit all edge and instrument the edges not in MST.
+// Visitor class that finds all indirect call sites.
+struct PGOIndirectCallSiteVisitor
+    : public InstVisitor<PGOIndirectCallSiteVisitor> {
+  std::vector<CallInst *> IndirectCallInsts;
+  PGOIndirectCallSiteVisitor() {}
+
+  void visitCallInst(CallInst &I) {
+    CallSite CS(&I);
+    if (CS.getCalledFunction() || !CS.getCalledValue())
+      return;
+    IndirectCallInsts.push_back(&I);
+  }
+};
+
+// Visit all edge and instrument the edges not in MST, and do value profiling.
 // Critical edges will be split.
 static void instrumentOneFunc(Function &F, Module *M,
                               BranchProbabilityInfo *BPI,
                               BlockFrequencyInfo *BFI) {
   unsigned NumCounters = 0;
   FuncPGOInstrumentation<PGOEdge, BBInfo> FuncInfo(F, true, BPI, BFI);
   for (auto &E : FuncInfo.MST.AllEdges) {
     if (!E->InMST && !E->Removed)
       NumCounters++;
   }
 
   uint32_t I = 0;
+  Type *I8PtrTy = Type::getInt8PtrTy(M->getContext());
   for (auto &E : FuncInfo.MST.AllEdges) {
     BasicBlock *InstrBB = FuncInfo.getInstrBB(E.get());
     if (!InstrBB)
       continue;
 
     IRBuilder<> Builder(InstrBB, InstrBB->getFirstInsertionPt());
     assert(Builder.GetInsertPoint() != InstrBB->end() &&
            "Cannot get the Instrumentation point");
-    Type *I8PtrTy = Type::getInt8PtrTy(M->getContext());
     Builder.CreateCall(
         Intrinsic::getDeclaration(M, Intrinsic::instrprof_increment),
         {llvm::ConstantExpr::getBitCast(FuncInfo.FuncNameVar, I8PtrTy),
          Builder.getInt64(FuncInfo.FunctionHash), Builder.getInt32(NumCounters),
          Builder.getInt32(I++)});
   }
+
+  if (DisableValueProfiling)
+    return;
+
+  unsigned NumIndirectCallSites = 0;
+  PGOIndirectCallSiteVisitor ICV;
+  ICV.visit(F);
+  for (auto &I : ICV.IndirectCallInsts) {
+    CallSite CS(I);
+    Value *Callee = CS.getCalledValue();
+    DEBUG(dbgs() << "Instrument one indirect call: CallSite Index = "
+                 << NumIndirectCallSites << "\n");
+    IRBuilder<> Builder(I);
+    assert(Builder.GetInsertPoint() != I->getParent()->end() &&
+           "Cannot get the Instrumentation point");
+    Builder.CreateCall(
+        Intrinsic::getDeclaration(M, Intrinsic::instrprof_value_profile),
+        {llvm::ConstantExpr::getBitCast(FuncInfo.FuncNameVar, I8PtrTy),
+         Builder.getInt64(FuncInfo.FunctionHash),
+         Builder.CreatePtrToInt(Callee, Builder.getInt64Ty()),
+         Builder.getInt32(llvm::InstrProfValueKind::IPVK_IndirectCallTarget),
+         Builder.getInt32(NumIndirectCallSites++)});
+  }
+  NumOfPGOICall += NumIndirectCallSites;
 }
 
 // This class represents a CFG edge in profile use compilation.
 struct PGOUseEdge : public PGOEdge {
   bool CountValid;
   uint64_t CountValue;
   PGOUseEdge(const BasicBlock *Src, const BasicBlock *Dest, unsigned W = 1)
       : PGOEdge(Src, Dest, W), CountValid(false), CountValue(0) {}

llvm/trunk/test/Transforms/PGOProfile/indirect_call_profile.ll

+; RUN: opt < %s -pgo-instr-gen -S | FileCheck %s --check-prefix=GEN
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@bar = external global void ()*, align 8
+; GEN: @__profn_foo = private constant [3 x i8] c"foo"
+
+define void @foo() {
+entry:
+; GEN: entry:
+; GEN-NEXT: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 12884901887, i32 1, i32 0)
+  %tmp = load void ()*, void ()** @bar, align 8
+; GEN: [[ICALL_TARGET:%[0-9]+]] = ptrtoint void ()* %tmp to i64
+; GEN-NEXT: call void @llvm.instrprof.value.profile(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 12884901887, i64 [[ICALL_TARGET]], i32 0, i32 0)
+  call void %tmp()
+  ret void
+}