Fix the samplepgo indirect call promotion bug: we should not promote a direct call.

[danielcdh]

Summary

Checking CS.getCalledFunction() == nullptr does not necessary indicate indirect call. We also need to check if CS.getCalledValue() is not a constant.

[danielcdh]

rebase

[davidxl]

lib/Transforms/IPO/SampleProfile.cpp

638

Refactor the code in llvm/Analysis/IndirectCallSiteVisitor.h and reuse the indirect call site query routine.

[danielcdh]

update

[davidxl]

include/llvm/IR/CallSite.h

116

check if getCalledValue nullness?

121

getCalledValue constant case?

[danielcdh]

update

[danielcdh]

include/llvm/IR/CallSite.h

121

I used "stripPointerCasts" on getCalledValue() so that this is not needed any more.

[davidxl]

include/llvm/IR/CallSite.h

121

how is it so?

[danielcdh]

include/llvm/IR/CallSite.h

121

stripPointerCasts comment says "Strip off pointer casts, all-zero GEPs, and aliases.", so for the constants (alias), it will give the actual function that it aliases to.

[danielcdh]

update

[davidxl]

lgtm.

include/llvm/Analysis/IndirectCallSiteVisitor.h

 namespace llvm {
 // Visitor class that finds all indirect call sites.
 struct PGOIndirectCallSiteVisitor
     : public InstVisitor<PGOIndirectCallSiteVisitor> {
   std::vector<Instruction *> IndirectCallInsts;
   PGOIndirectCallSiteVisitor() {}
 
   void visitCallSite(CallSite CS) {
-    if (CS.getCalledFunction() || !CS.getCalledValue())
-      return;
-    Instruction *I = CS.getInstruction();
-    if (CallInst *CI = dyn_cast<CallInst>(I)) {
-      if (CI->isInlineAsm())
-        return;
-    }
-    if (isa<Constant>(CS.getCalledValue()))
-      return;
-    IndirectCallInsts.push_back(I);
+    if (CS.isIndirectCall())
+      IndirectCallInsts.push_back(CS.getInstruction());
   }
 };
 
 // Helper function that finds all indirect call sites.
 static inline std::vector<Instruction *> findIndirectCallSites(Function &F) {
   PGOIndirectCallSiteVisitor ICV;
   ICV.visit(F);
   return ICV.IndirectCallInsts;
 }
 }

include/llvm/IR/CallSite.h

public:
 
   /// getCalledFunction - Return the function being called if this is a direct
   /// call, otherwise return null (if it's an indirect call).
   ///
   FunTy *getCalledFunction() const {
     return dyn_cast<FunTy>(getCalledValue());
   }
 
+  /// Returns true if the callsite is an indirect call
+  bool isIndirectCall() const {
+    Value *V = getCalledValue();
+    if (!V)
+      return false;
+    if (isa<FunTy>(V) || isa<Constant>(V))
+      return false;
+    if (CallInst *CI = dyn_cast<CallInst>(getInstruction())) {
+      if (CI->isInlineAsm())
+        return false;
+    }
+    return true;
+  }
+
   /// setCalledFunction - Set the callee to the specified value.
   ///
   void setCalledFunction(Value *V) {
     assert(getInstruction() && "Not a call or invoke instruction!");
     *getCallee() = V;
   }
 
   /// Return the intrinsic ID of the intrinsic called by this CallSite,

lib/Transforms/IPO/SampleProfile.cpp

for (auto &BB : F) {
       if (Hot) {
         CIS.insert(CIS.begin(), Candidates.begin(), Candidates.end());
       }
     }
     for (auto I : CIS) {
       InlineFunctionInfo IFI(nullptr, ACT ? &GetAssumptionCache : nullptr);
       Function *CalledFunction = CallSite(I).getCalledFunction();
       Instruction *DI = I;
-      if (!CalledFunction && !PromotedInsns.count(I)) {
+      if (!CalledFunction && !PromotedInsns.count(I) &&
+          CallSite(I).isIndirectCall()) {
         auto CalleeFunctionName = findCalleeFunctionSamples(*I)->getName();
         const char *Reason = "Callee function not available";
         CalledFunction = F.getParent()->getFunction(CalleeFunctionName);
         if (CalledFunction && isLegalToPromote(I, CalledFunction, &Reason)) {
           // The indirect target was promoted and inlined in the profile, as a
           // result, we do not have profile info for the branch probability.
           // We set the probability to 80% taken to indicate that the static
           // call is likely taken.

test/Transforms/SampleProfile/Inputs/indirect-call.prof

 test:63067:0
  4: 3345 _Z3barv:1398 _Z3foov:2059
 test_inline:3000:0
  5: foo_inline:3000
   1: 3000
 test_noinline:3000:0
  5: foo_noinline:3000
   1: 3000
+test_direct:3000:0
+ 5: foo_direct:3000
+  1: 3000

test/Transforms/SampleProfile/indirect-call.ll

 define i32* @foo_inline(i32* %x) !dbg !3 {
   ret i32* %x
 }
 
 define i32 @foo_noinline(i32 %x) !dbg !3 {
   ret i32 %x
 }
 
+define void @foo_direct() !dbg !3 {
+  ret void
+}
+
+; CHECK-LABEL: @test_direct
+; We should not promote a direct call.
+define void @test_direct() !dbg !3 {
+; CHECK-NOT: icmp
+; CHECK: call
+  call void @foo_alias(), !dbg !5
+  ret void
+}
+
+@foo_alias = alias void (), void ()* @foo_direct
+
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!2}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1)
 !1 = !DIFile(filename: "test.cc", directory: "/")
 !2 = !{i32 2, !"Debug Info Version", i32 3}
 !3 = distinct !DISubprogram(name: "test", scope: !1, file: !1, line: 1, unit: !0)
 !4 = !DILocation(line: 5, scope: !3)
 !5 = !DILocation(line: 6, scope: !3)
 ; CHECK: ![[PROF]] = !{!"VP", i32 0, i64 3457, i64 9191153033785521275, i64 2059, i64 -1069303473483922844, i64 1398}