Clarify definition of "indirect call" in CallSite

[scott.linder]

Summary

Attempt to clarify the definition of "indirect call" in LLVM, and add a function to look through trivial casts in a call instruction.

Rename and invert semantics of "isIndirectCall" to "isConstant". I don't like the new name, but I'm not sure how else to distinguish it from "direct/indirect".

[sbc100]

lib/Transforms/InstCombine/InstCombineCalls.cpp

4242

These auto s should probably be replaced with actual types now there is no type on the RHS.

[scott.linder]

Address feedback

[scott.linder]

Remove update of dyn_cast in WebAssemblyFixFunctionBitcasts which does not use CallSite

[efriedma]

include/llvm/IR/CallSite.h

110

Maybe it would be better to avoid "direct" and "indirect"...? Maybe just say "Return the function being called if the callee Value is a Function".

120

This isn't what isIndirectCall actually implements...

120

The new function seems nice.

[scott.linder]

include/llvm/IR/CallSite.h

110

One issue is that "direct" and "indirect" are used in similar ways in other places. For example CallGraphSCCPass refers to "devirtualizing" calls which are of the form call bitcast @func because getCalledFunction stops returning nullptr.

I can attempt to find and update all of these comments as well if we think fixing this terminology is useful. I am personally still confused about how these terms are used, and used inconsistently (as you point out below, getCalledFunction and isIndirectCall disagree on the term and they are in the same class).

120

Right, I completely missed that a BitCast is a Constant, and also did not mention the specific check for InlineAsm.

When can getCalledValue return nullptr? And is this definition "correct" for LLVM (e.g. an indirect call is any call which is not one of these cases)?

[efriedma]

include/llvm/IR/CallSite.h

120

getCalledValue never returns nullptr.

Assuming you meant getCalledFunction, it only returns non-null if the callee is a Function.

isIndirectCall() isn't really used by anything outside of profiling infrastructure, so the implementation doesn't really affect much.

Maybe it's best to try to avoid defining "direct" and "indirect" calls, and just directly describe what the functions return.

[scott.linder]

include/llvm/IR/CallSite.h

120

I meant getCalledValue; the first check in isIndirectCall is:

const Value *V = getCalledValue();
if (!V)
      return false;

Which is redundant if getCalledValue can't return nullptr.

The point of this patch is to clarify the definition, so having a function called isIndirectCall which answers a completely different question doesn't really move anything forward. I don't have a good grip on the right way to define this, though, as the term gets used in a number of places in a way which implies it has multiple definitions. I think ideally "indirect" would actually be defined as it is implemented here, for isIndirectCall, because as far as I know there is no way to introduce a non-constant value into a Constant expression.

[efriedma]

include/llvm/IR/CallSite.h

120

Which is redundant if getCalledValue can't return nullptr.

Yes, it's redundant.

---

isIndirectCall() doesn't really implement what anything in LLVM treats as an indirect call outside of the profiling infrastructure; it should probably be renamed.

[scott.linder]

I still think disambiguating "indirect call" in LLVM is worthwhile, so here is another attempt at addressing feedback.

[efriedma]

Please rebase on top of https://reviews.llvm.org/D52537 .

[scott.linder]

In D52894#1289265, @efriedma wrote:

Please rebase on top of https://reviews.llvm.org/D52537 .

Is CallSite being superceded by CallBase? Also, from the linked patch it seems like my assumption that getCalledValue() cannot return null is invalid? Is this true for both CallSite and CallBase?

[efriedma]

Is CallSite being superceded by CallBase?

Oh, oops, wasn't looking that closely. Eventually, yes, as part of the terminator cleanup, so we should probably try to make the two agree in the meantime so the transition is straightforward.

Also, from the linked patch it seems like my assumption that getCalledValue() cannot return null is invalid? Is this true for both CallSite and CallBase?

Well, for "valid" IR (that's accepted by the verifier), getCalledValue() is never null. But some passes temporarily null out operands as part of certain algorithms, like erasing dead code, and a small subset of IR APIs work for IR in that state. Not sure getCalledValue()/getCalledFunction() should be in that subset, but it doesn't matter much either way.

include/llvm/Analysis/IndirectCallSiteVisitor.h

-//===-- IndirectCallSiteVisitor.h - indirect call-sites visitor -----------===//
-//
-//                      The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//
-// This file implements defines a visitor class and a helper function that find
-// all indirect call-sites in a function.
-
-#ifndef LLVM_ANALYSIS_INDIRECTCALLSITEVISITOR_H
-#define LLVM_ANALYSIS_INDIRECTCALLSITEVISITOR_H
-
-#include "llvm/IR/InstVisitor.h"
-#include <vector>
-
-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.isIndirectCall())
-      IndirectCallInsts.push_back(CS.getInstruction());
-  }
-};
-
-// Helper function that finds all indirect call sites.
-inline std::vector<Instruction *> findIndirectCallSites(Function &F) {
-  PGOIndirectCallSiteVisitor ICV;
-  ICV.visit(F);
-  return ICV.IndirectCallInsts;
-}
-}
-
-#endif

include/llvm/Analysis/NonConstantCallSiteVisitor.h

+//===-- NonConstantCallSiteVisitor.h - non-constant call site visitor -----===//
+//
+//                      The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines and implements a visitor class and a helper function that
+// find all non-constant call sites in a function.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ANALYSIS_NONCONSTANTCALLSITEVISITOR_H
+#define LLVM_ANALYSIS_NONCONSTANTCALLSITEVISITOR_H
+
+#include "llvm/IR/InstVisitor.h"
+#include <vector>
+
+namespace llvm {
+// Visitor class that finds all non-constant call sites.
+struct PGONonConstantCallSiteVisitor
+    : public InstVisitor<PGONonConstantCallSiteVisitor> {
+  std::vector<Instruction *> NonConstantCallInsts;
+  PGONonConstantCallSiteVisitor() {}
+
+  void visitCallSite(CallSite CS) {
+    if (!CS.isConstant())
+      NonConstantCallInsts.push_back(CS.getInstruction());
+  }
+};
+
+// Helper function that finds all non-constant call sites.
+inline std::vector<Instruction *> findNonConstantCallSites(Function &F) {
+  PGONonConstantCallSiteVisitor ICV;
+  ICV.visit(F);
+  return ICV.NonConstantCallInsts;
+}
+}
+
+#endif

include/llvm/IR/CallSite.h

public:
 
   InstrTy *getInstruction() const { return I.getPointer(); }
   InstrTy *operator->() const { return I.getPointer(); }
   explicit operator bool() const { return I.getPointer(); }
 
   /// Get the basic block containing the call site.
   BBTy* getParent() const { return getInstruction()->getParent(); }
 
-  /// Return the pointer to function that is being called.
+  /// Return the pointer to the value that is being called.
+  ///
+  /// getCalledValue never returns null.
   ValTy *getCalledValue() const {
     assert(getInstruction() && "Not a call or invoke instruction!");
     return *getCallee();
   }
 
   /// Return the function being called if this is a direct call, otherwise
   /// return null (if it's an indirect call).
+  ///
+  /// A direct call is a call to a FunTy. An indirect call is a call to
+  /// anything which is not a FunTy (e.g. a bitcast of a FunTy).
   FunTy *getCalledFunction() const {
     return dyn_cast<FunTy>(getCalledValue());
   }
 
-  /// Return true if the callsite is an indirect call.
-  bool isIndirectCall() const {
+  /// Return the function being called if this is either a direct call or an
+  /// indirect call to a cast of a FunTy, otherwise return null.
+  ///
+  /// See getCalledFunction for the definitions of direct and indirect calls.
+  FunTy *getCalledFunctionStripCasts() const {
+    return dyn_cast<FunTy>(getCalledValue()->stripPointerCasts());
+  }
+
+  /// Return true if the call site is "constant". A call site is constant if it
+  /// is to a function, constant, or inline assembly.
+  bool isConstant() const {
     const Value *V = getCalledValue();
-    if (!V)
-      return false;
     if (isa<FunTy>(V) || isa<Constant>(V))
-      return false;
+      return true;
     if (const CallInst *CI = dyn_cast<CallInst>(getInstruction())) {
       if (CI->isInlineAsm())
-        return false;
-    }
         return true;
     }
+    return false;
+  }
 
   /// 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/Analysis/IndirectCallPromotionAnalysis.cpp

 // Helper methods for identifying profitable indirect call promotion
 // candidates for an instruction when the indirect-call value profile metadata
 // is available.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Analysis/IndirectCallPromotionAnalysis.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/Analysis/IndirectCallSiteVisitor.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/Debug.h"
 #include <string>

lib/CodeGen/WinEHPrepare.cpp

for (BasicBlock *BB : BlocksInFunclet) {
         Value *FuncletBundleOperand = nullptr;
         if (auto BU = CS.getOperandBundle(LLVMContext::OB_funclet))
           FuncletBundleOperand = BU->Inputs.front();
 
         if (FuncletBundleOperand == FuncletPad)
           continue;
 
         // Skip call sites which are nounwind intrinsics or inline asm.
-        auto *CalledFn =
-            dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
+        Function *CalledFn = CS.getCalledFunctionStripCasts();
         if (CalledFn && ((CalledFn->isIntrinsic() && CS.doesNotThrow()) ||
                          CS.isInlineAsm()))
           continue;
 
         // This call site was not part of this funclet, remove it.
         if (CS.isInvoke()) {
           // Remove the unwind edge if it was an invoke.
           removeUnwindEdge(BB);

lib/IR/Verifier.cpp

void Verifier::verifyCallSite(CallSite CS) {
   AttributeList Attrs = CS.getAttributes();
 
   Assert(verifyAttributeCount(Attrs, CS.arg_size()),
          "Attribute after last parameter!", I);
 
   if (Attrs.hasAttribute(AttributeList::FunctionIndex, Attribute::Speculatable)) {
     // Don't allow speculatable on call sites, unless the underlying function
     // declaration is also speculatable.
-    Function *Callee
-      = dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
+    Function *Callee = CS.getCalledFunctionStripCasts();
     Assert(Callee && Callee->isSpeculatable(),
            "speculatable attribute may not apply to call sites", I);
   }
 
   // Verify call attributes.
   verifyFunctionAttrs(FTy, Attrs, I);
 
   // Conservatively check the inalloca argument.

lib/Target/AMDGPU/AMDGPUFixFunctionBitcasts.cpp

class AMDGPUFixFunctionBitcasts final
   bool runOnModule(Module &M) override;
 
   bool Modified;
 
 public:
   void visitCallSite(CallSite CS) {
     if (CS.getCalledFunction())
       return;
-    auto Callee = dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
+    auto Callee = CS.getCalledFunctionStripCasts();
     if (Callee && isLegalToPromote(CS, Callee)) {
       promoteCall(CS, Callee);
       Modified = true;
     }
   }
 
   static char ID;
   AMDGPUFixFunctionBitcasts() : ModulePass(ID) {}

lib/Transforms/IPO/SampleProfile.cpp

ErrorOr<uint64_t> SampleProfileLoader::getInstWeight(const Instruction &Inst) {
   if (isa<BranchInst>(Inst) || isa<IntrinsicInst>(Inst))
     return std::error_code();
 
   // If a direct call/invoke instruction is inlined in profile
   // (findCalleeFunctionSamples returns non-empty result), but not inlined here,
   // it means that the inlined callsite has no sample, thus the call
   // instruction should have 0 count.
   if ((isa<CallInst>(Inst) || isa<InvokeInst>(Inst)) &&
-      !ImmutableCallSite(&Inst).isIndirectCall() &&
+      ImmutableCallSite(&Inst).isConstant() &&
       findCalleeFunctionSamples(Inst))
     return 0;
 
   const DILocation *DIL = DLoc;
   uint32_t LineOffset = FunctionSamples::getOffset(DIL);
   uint32_t Discriminator = DIL->getBaseDiscriminator();
   ErrorOr<uint64_t> R = FS->findSamplesAt(LineOffset, Discriminator);
   if (R) {
for (auto &BB : F) {
         CIS.insert(CIS.begin(), Candidates.begin(), Candidates.end());
       }
     }
     for (auto I : CIS) {
       Function *CalledFunction = CallSite(I).getCalledFunction();
       // Do not inline recursive calls.
       if (CalledFunction == &F)
         continue;
-      if (CallSite(I).isIndirectCall()) {
+      if (!CallSite(I).isConstant()) {
         if (PromotedInsns.count(I))
           continue;
         uint64_t Sum;
         for (const auto *FS : findIndirectCallFunctionSamples(*I, Sum)) {
           if (IsThinLTOPreLink) {
             FS->findInlinedFunctions(InlinedGUIDs, F.getParent(),
                                      PSI->getOrCompHotCountThreshold());
             continue;

lib/Transforms/InstCombine/InstCombineCalls.cpp

Instruction *InstCombiner::visitCallSite(CallSite CS) {
   }
 
   return Changed ? CS.getInstruction() : nullptr;
 }
 
 /// If the callee is a constexpr cast of a function, attempt to move the cast to
 /// the arguments of the call/invoke.
 bool InstCombiner::transformConstExprCastCall(CallSite CS) {
-  auto *Callee = dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
+  Function *Callee = CS.getCalledFunctionStripCasts();
   if (!Callee)
     return false;
 
   // If this is a call to a thunk function, don't remove the cast. Thunks are
   // used to transparently forward all incoming parameters and outgoing return
   // values, so it's important to leave the cast in place.
   if (Callee->hasFnAttribute("thunk"))
     return false;

lib/Transforms/Instrumentation/CGProfile.cpp

for (auto &F : M) {
     for (auto &BB : F) {
       Optional<uint64_t> BBCount = BFI.getBlockProfileCount(&BB);
       if (!BBCount)
         continue;
       for (auto &I : BB) {
         CallSite CS(&I);
         if (!CS)
           continue;
-        if (CS.isIndirectCall()) {
+        if (!CS.isConstant()) {
           InstrProfValueData ValueData[8];
           uint32_t ActualNumValueData;
           uint64_t TotalC;
           if (!getValueProfDataFromInst(*CS.getInstruction(),
                                         IPVK_IndirectCallTarget, 8, ValueData,
                                         ActualNumValueData, TotalC))
             continue;
           for (const auto &VD :

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Analysis/IndirectCallPromotionAnalysis.h"
-#include "llvm/Analysis/IndirectCallSiteVisitor.h"
+#include "llvm/Analysis/NonConstantCallSiteVisitor.h"
 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/Analysis/ProfileSummaryInfo.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
uint32_t ICallPromotionFunc::tryToPromote(
   return NumPromoted;
 }
 
 // Traverse all the indirect-call callsite and get the value profile
 // annotation to perform indirect-call promotion.
 bool ICallPromotionFunc::processFunction(ProfileSummaryInfo *PSI) {
   bool Changed = false;
   ICallPromotionAnalysis ICallAnalysis;
-  for (auto &I : findIndirectCallSites(F)) {
+  for (auto &I : findNonConstantCallSites(F)) {
     uint32_t NumVals, NumCandidates;
     uint64_t TotalCount;
     auto ICallProfDataRef = ICallAnalysis.getPromotionCandidatesForInstruction(
         I, NumVals, TotalCount, NumCandidates);
     if (!NumCandidates ||
         (PSI && PSI->hasProfileSummary() && !PSI->isHotCount(TotalCount)))
       continue;
     auto PromotionCandidates = getPromotionCandidatesForCallSite(

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Analysis/BlockFrequencyInfo.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/Analysis/CFG.h"
-#include "llvm/Analysis/IndirectCallSiteVisitor.h"
+#include "llvm/Analysis/NonConstantCallSiteVisitor.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Comdat.h"
 #include "llvm/IR/Constant.h"
FuncPGOInstrumentation(
       BlockFrequencyInfo *BFI = nullptr)
       : F(Func), ComdatMembers(ComdatMembers), ValueSites(IPVK_Last + 1),
         SIVisitor(Func), MIVisitor(Func), MST(F, BPI, BFI) {
     // This should be done before CFG hash computation.
     SIVisitor.countSelects(Func);
     MIVisitor.countMemIntrinsics(Func);
     NumOfPGOSelectInsts += SIVisitor.getNumOfSelectInsts();
     NumOfPGOMemIntrinsics += MIVisitor.getNumOfMemIntrinsics();
-    ValueSites[IPVK_IndirectCallTarget] = findIndirectCallSites(Func);
+    ValueSites[IPVK_IndirectCallTarget] = findNonConstantCallSites(Func);
     ValueSites[IPVK_MemOPSize] = MIVisitor.findMemIntrinsics(Func);
 
     FuncName = getPGOFuncName(F);
     computeCFGHash();
     if (!ComdatMembers.empty())
       renameComdatFunction();
     LLVM_DEBUG(dumpInfo("after CFGMST"));
 

lib/Transforms/Utils/InlineFunction.cpp

for (Function::iterator BB = FirstNewBlock->getIterator(),
       SmallVector<OperandBundleDef, 1> OpBundles;
       for (BasicBlock::iterator BBI = BB->begin(), E = BB->end(); BBI != E;) {
         Instruction *I = &*BBI++;
         CallSite CS(I);
         if (!CS)
           continue;
 
         // Skip call sites which are nounwind intrinsics.
-        auto *CalledFn =
-            dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
+        Function *CalledFn = CS.getCalledFunctionStripCasts();
         if (CalledFn && CalledFn->isIntrinsic() && CS.doesNotThrow())
           continue;
 
         // Skip call sites which already have a "funclet" bundle.
         if (CS.getOperandBundle(LLVMContext::OB_funclet))
           continue;
 
         CS.getOperandBundlesAsDefs(OpBundles);