[Verifier] Rephrase visitIntrinsicFunctionCall in terms of CallSites

[reames]

Summary

The motivation is to remove a special case for invokable statepoints and share the code for intrinsic verification for CallInst and InvokeInst.

The code change is intentionally very mechanical. I left off variable renaming - which will happen in a follow on change - to reduce the textual diff to simplify review. The only part that really needs review is a) whether adding all the methods to CallSite is reasonable to make it closer to a drop in replacement for CallInst, and b) whether extending Write to support CallSites directly is reasonable. (I used the CallSite * to reduce the textual diff for converting a function from CallInst to CallSite. I'm open to changing this, but think the current version is worthwhile, even if ugly.)

[reames]

ping

[sanjoy]

I think this is okay to check in, modulo three inline comments (and that the s/CI/CS fix happens post-commit). However, it would be nice if someone else who's touched CallSite.h also takes a look.

include/llvm/IR/CallSite.h

84

I think the type should be a template parameter so that we return a const BasicBlock * from an ImmutableCallSite and a BasicBlock * from a normal CallSite.

200

Should be ValTy *.

lib/IR/Verifier.cpp

108

Why not just Assert(..., CS.getInstruction()) instead?

include/llvm/IR/CallSite.h

public:
   /// isInvoke - true if a InvokeInst is enclosed.
   ///
   bool isInvoke() const { return getInstruction() && !I.getInt(); }
 
   InstrTy *getInstruction() const { return I.getPointer(); }
   InstrTy *operator->() const { return I.getPointer(); }
   explicit operator bool() const { return I.getPointer(); }
 
+  BasicBlock* getParent() { return getInstruction()->getParent(); }
+  const BasicBlock* getParent() const { return getInstruction()->getParent(); }
+
   /// getCalledValue - Return the pointer to function that is being called.
   ///
   ValTy *getCalledValue() const {
     assert(getInstruction() && "Not a call or invoke instruction!");
     return *getCallee();
   }
 
   /// getCalledFunction - Return the function being called if this is a direct
public:
   /// list for a call site.
   IterTy arg_begin() const {
     assert(getInstruction() && "Not a call or invoke instruction!");
     // Skip non-arguments
     return (*this)->op_begin();
   }
 
   IterTy arg_end() const { return (*this)->op_end() - getArgumentEndOffset(); }
+
+  iterator_range<IterTy> args() {
+    return iterator_range<IterTy>(arg_begin(), arg_end());
+  }
+
   bool arg_empty() const { return arg_end() == arg_begin(); }
   unsigned arg_size() const { return unsigned(arg_end() - arg_begin()); }
 
   /// getType - Return the type of the instruction that generated this call site
   ///
   Type *getType() const { return (*this)->getType(); }
 
   /// getCaller - Return the caller function for this call site

 
 #define CALLSITE_DELEGATE_SETTER(METHOD) \
   InstrTy *II = getInstruction();    \
   if (isCall())                          \
     cast<CallInst>(II)->METHOD;          \
   else                                   \
     cast<InvokeInst>(II)->METHOD
 
+  unsigned getNumArgOperands() const {
+    CALLSITE_DELEGATE_GETTER(getNumArgOperands());
+  }
+
+  Value *getArgOperand(unsigned i) const { 
+    CALLSITE_DELEGATE_GETTER(getArgOperand(i));
+  }
+
+  bool isInlineAsm() const { 
+    if (isCall())
+      return cast<CallInst>(getInstruction())->isInlineAsm();
+    return false;
+  }
+
   /// getCallingConv/setCallingConv - get or set the calling convention of the
   /// call.
   CallingConv::ID getCallingConv() const {
     CALLSITE_DELEGATE_GETTER(getCallingConv());
   }
   void setCallingConv(CallingConv::ID CC) {
     CALLSITE_DELEGATE_SETTER(setCallingConv(CC));
   }

lib/IR/Verifier.cpp

if (!V)
       return;
     if (isa<Instruction>(V)) {
       OS << *V << '\n';
     } else {
       V->printAsOperand(OS, true, M);
       OS << '\n';
     }
   }
+  void Write(const CallSite *CS) {
+    if (!CS) 
+      return;
+    Write(CS->getInstruction());
+  }
 
   void Write(const Metadata *MD) {
     if (!MD)
       return;
     MD->print(OS, M);
     OS << '\n';
   }
 
#include "llvm/IR/Metadata.def"
   void visitTerminatorInst(TerminatorInst &I);
   void visitBranchInst(BranchInst &BI);
   void visitReturnInst(ReturnInst &RI);
   void visitSwitchInst(SwitchInst &SI);
   void visitIndirectBrInst(IndirectBrInst &BI);
   void visitSelectInst(SelectInst &SI);
   void visitUserOp1(Instruction &I);
   void visitUserOp2(Instruction &I) { visitUserOp1(I); }
-  void visitIntrinsicFunctionCall(Intrinsic::ID ID, CallInst &CI);
+  void visitIntrinsicFunctionCall(Intrinsic::ID ID, CallSite CS);
   template <class DbgIntrinsicTy>
   void visitDbgIntrinsic(StringRef Kind, DbgIntrinsicTy &DII);
   void visitAtomicCmpXchgInst(AtomicCmpXchgInst &CXI);
   void visitAtomicRMWInst(AtomicRMWInst &RMWI);
   void visitFenceInst(FenceInst &FI);
   void visitAllocaInst(AllocaInst &AI);
   void visitExtractValueInst(ExtractValueInst &EVI);
   void visitInsertValueInst(InsertValueInst &IVI);
void Verifier::VerifyCallSite(CallSite CS) {
   if (CS.getCalledFunction() == nullptr ||
       !CS.getCalledFunction()->getName().startswith("llvm.")) {
     for (FunctionType::param_iterator PI = FTy->param_begin(),
            PE = FTy->param_end(); PI != PE; ++PI)
       Assert(!(*PI)->isMetadataTy(),
              "Function has metadata parameter but isn't an intrinsic", I);
   }
 
+  if (Function *F = CS.getCalledFunction())
+    if (Intrinsic::ID ID = (Intrinsic::ID)F->getIntrinsicID())
+      visitIntrinsicFunctionCall(ID, CS);
+
   visitInstruction(*I);
 }
 
 /// Two types are "congruent" if they are identical, or if they are both pointer
 /// types with different pointee types and the same address space.
 static bool isTypeCongruent(Type *L, Type *R) {
   if (L == R)
     return true;
Assert(!Ret->getReturnValue() || Ret->getReturnValue() == RetVal,
          "musttail call result must be returned", Ret);
 }
 
 void Verifier::visitCallInst(CallInst &CI) {
   VerifyCallSite(&CI);
 
   if (CI.isMustTailCall())
     verifyMustTailCall(CI);
-
-  if (Function *F = CI.getCalledFunction())
-    if (Intrinsic::ID ID = (Intrinsic::ID)F->getIntrinsicID())
-      visitIntrinsicFunctionCall(ID, CI);
 }
 
 void Verifier::visitInvokeInst(InvokeInst &II) {
   VerifyCallSite(&II);
 
   // Verify that there is a landingpad instruction as the first non-PHI
   // instruction of the 'unwind' destination.
   Assert(II.getUnwindDest()->isLandingPad(),
          "The unwind destination does not have a landingpad instruction!", &II);
 
-  if (Function *F = II.getCalledFunction())
-    // TODO: Ideally we should use visitIntrinsicFunction here. But it uses
-    //       CallInst as an input parameter. It not woth updating this whole
-    //       function only to support statepoint verification.
-    if (F->getIntrinsicID() == Intrinsic::experimental_gc_statepoint)
-      VerifyStatepoint(ImmutableCallSite(&II));
-
   visitTerminatorInst(II);
 }
 
 /// visitBinaryOperator - Check that both arguments to the binary operator are
 /// of the same type!
 ///
 void Verifier::visitBinaryOperator(BinaryOperator &B) {
   Assert(B.getOperand(0)->getType() == B.getOperand(1)->getType(),
Verifier::VerifyIntrinsicIsVarArg(bool isVarArg,
   if (D.Kind == IITDescriptor::VarArg)
     return !isVarArg;
 
   return true;
 }
 
 /// visitIntrinsicFunction - Allow intrinsics to be verified in different ways.
 ///
-void Verifier::visitIntrinsicFunctionCall(Intrinsic::ID ID, CallInst &CI) {
+void Verifier::visitIntrinsicFunctionCall(Intrinsic::ID ID, CallSite CI) {
   Function *IF = CI.getCalledFunction();
   Assert(IF->isDeclaration(), "Intrinsic functions should never be defined!",
          IF);
 
   // Verify that the intrinsic prototype lines up with what the .td files
   // describe.
   FunctionType *IFTy = IF->getFunctionType();
   bool IsVarArg = IFTy->isVarArg();
case Intrinsic::cttz:  // llvm.cttz
     Assert(isa<ConstantInt>(CI.getArgOperand(1)),
            "is_zero_undef argument of bit counting intrinsics must be a "
            "constant int",
            &CI);
     break;
   case Intrinsic::dbg_declare: // llvm.dbg.declare
     Assert(isa<MetadataAsValue>(CI.getArgOperand(0)),
            "invalid llvm.dbg.declare intrinsic call 1", &CI);
-    visitDbgIntrinsic("declare", cast<DbgDeclareInst>(CI));
+    visitDbgIntrinsic("declare", cast<DbgDeclareInst>(*CI.getInstruction()));
     break;
   case Intrinsic::dbg_value: // llvm.dbg.value
-    visitDbgIntrinsic("value", cast<DbgValueInst>(CI));
+    visitDbgIntrinsic("value", cast<DbgValueInst>(*CI.getInstruction()));
     break;
   case Intrinsic::memcpy:
   case Intrinsic::memmove:
   case Intrinsic::memset: {
     ConstantInt *AlignCI = dyn_cast<ConstantInt>(CI.getArgOperand(3));
     Assert(AlignCI,
            "alignment argument of memory intrinsics must be a constant int",
            &CI);
case Intrinsic::invariant_end:
     break;
 
   case Intrinsic::frameescape: {
     BasicBlock *BB = CI.getParent();
     Assert(BB == &BB->getParent()->front(),
            "llvm.frameescape used outside of entry block", &CI);
     Assert(!SawFrameEscape,
            "multiple calls to llvm.frameescape in one function", &CI);
-    for (Value *Arg : CI.arg_operands()) {
+    for (Value *Arg : CI.args()) {
       if (isa<ConstantPointerNull>(Arg))
         continue; // Null values are allowed as placeholders.
       auto *AI = dyn_cast<AllocaInst>(Arg->stripPointerCasts());
       Assert(AI && AI->isStaticAlloca(),
              "llvm.frameescape only accepts static allocas", &CI);
     }
     FrameEscapeInfo[BB->getParent()].first = CI.getNumArgOperands();
     SawFrameEscape = true;
void Verifier::visitIntrinsicFunctionCall(Intrinsic::ID ID, CallSite CI) {
   }
 
   case Intrinsic::experimental_gc_statepoint:
     Assert(!CI.isInlineAsm(),
            "gc.statepoint support for inline assembly unimplemented", &CI);
     Assert(CI.getParent()->getParent()->hasGC(),
            "Enclosing function does not use GC.", &CI);
 
-    VerifyStatepoint(ImmutableCallSite(&CI));
+    VerifyStatepoint(ImmutableCallSite(CI));
     break;
   case Intrinsic::experimental_gc_result_int:
   case Intrinsic::experimental_gc_result_float:
   case Intrinsic::experimental_gc_result_ptr:
   case Intrinsic::experimental_gc_result: {
     Assert(CI.getParent()->getParent()->hasGC(),
            "Enclosing function does not use GC.", &CI);
     // Are we tied to a statepoint properly?
else {
       // relocates of a call statepoint
       auto Token = CI.getArgOperand(0);
       Assert(isa<Instruction>(Token) && isStatepoint(cast<Instruction>(Token)),
              "gc relocate is incorrectly tied to the statepoint", &CI, Token);
     }
 
     // Verify rest of the relocate arguments
 
-    GCRelocateOperands ops(&CI);
+    GCRelocateOperands ops(CI);
     ImmutableCallSite StatepointCS(ops.getStatepoint());
 
     // Both the base and derived must be piped through the safepoint
     Value* Base = CI.getArgOperand(1);
     Assert(isa<ConstantInt>(Base),
            "gc.relocate operand #2 must be integer offset", &CI);
 
     Value* Derived = CI.getArgOperand(2);
Assert(GCParamArgsStart <= BaseIndex && BaseIndex < GCParamArgsEnd,
            "'gc parameters' section of the statepoint call",
            &CI);
     Assert(GCParamArgsStart <= DerivedIndex && DerivedIndex < GCParamArgsEnd,
            "gc.relocate: statepoint derived index doesn't fall within the "
            "'gc parameters' section of the statepoint call",
            &CI);
 
     // Assert that the result type matches the type of the relocated pointer
-    GCRelocateOperands Operands(&CI);
+    GCRelocateOperands Operands(CI);
     Assert(Operands.getDerivedPtr()->getType() == CI.getType(),
            "gc.relocate: relocating a pointer shouldn't change its type", &CI);
     break;
   }
   };
 }
 
 /// \brief Carefully grab the subprogram from a local scope.