[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 ↗

(On Diff #26768)

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 ↗

(On Diff #26768)

Should be ValTy *.

lib/IR/Verifier.cpp

108 ↗

(On Diff #26768)

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

llvm/trunk/include/llvm/IR/CallSite.h

 #include "llvm/IR/Instructions.h"
 
 namespace llvm {
 
 class CallInst;
 class InvokeInst;
 
 template <typename FunTy = const Function,
+          typename BBTy = const BasicBlock,
           typename ValTy = const Value,
           typename UserTy = const User,
           typename InstrTy = const Instruction,
           typename CallTy = const CallInst,
           typename InvokeTy = const InvokeInst,
           typename IterTy = User::const_op_iterator>
 class CallSiteBase {
 protected:
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(); }
 
+  /// Get the basic block containing the call site
+  BBTy* 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

 
 #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));
   }
private:
   IterTy getCallee() const {
     if (isCall()) // Skip Callee
       return cast<CallInst>(getInstruction())->op_end() - 1;
     else // Skip BB, BB, Callee
       return cast<InvokeInst>(getInstruction())->op_end() - 3;
   }
 };
 
-class CallSite : public CallSiteBase<Function, Value, User, Instruction,
-                                     CallInst, InvokeInst, User::op_iterator> {
+class CallSite : public CallSiteBase<Function, BasicBlock, Value, User,
+                                     Instruction, CallInst, InvokeInst,
+                                     User::op_iterator> {
 public:
   CallSite() {}
   CallSite(CallSiteBase B) : CallSiteBase(B) {}
   CallSite(CallInst *CI) : CallSiteBase(CI) {}
   CallSite(InvokeInst *II) : CallSiteBase(II) {}
   explicit CallSite(Instruction *II) : CallSiteBase(II) {}
   explicit CallSite(Value *V) : CallSiteBase(V) {}
 

llvm/trunk/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 = 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);
case Intrinsic::experimental_gc_relocate: {
     Assert(GCParamArgsStart <= DerivedIndex && DerivedIndex < GCParamArgsEnd,
            "gc.relocate: statepoint derived index doesn't fall within the "
            "'gc parameters' section of the statepoint call",
            &CI);
 
     // Relocated value must be a pointer type, but gc_relocate does not need to return the
     // same pointer type as the relocated pointer. It can be casted to the correct type later
     // if it's desired. However, they must have the same address space.
-    GCRelocateOperands Operands(&CI);
+    GCRelocateOperands Operands(CI);
     Assert(Operands.getDerivedPtr()->getType()->isPointerTy(),
            "gc.relocate: relocated value must be a gc pointer", &CI);
 
     // gc_relocate return type must be a pointer type, and is verified earlier in
     // VerifyIntrinsicType().
     Assert(cast<PointerType>(CI.getType())->getAddressSpace() ==
            cast<PointerType>(Operands.getDerivedPtr()->getType())->getAddressSpace(),
            "gc.relocate: relocating a pointer shouldn't change its address space", &CI);

llvm/trunk/test/CodeGen/WinEH/cppeh-prepared-catch.ll

 ; CHECK:        .long   ("$cppxdata$?f@@YAXXZ")@IMGREL
 
 
 define internal i8* @"\01?f@@YAXXZ.catch1"(i8*, i8*) #4 personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
 entry:
   %.i8 = call i8* @llvm.framerecover(i8* bitcast (void ()* @"\01?f@@YAXXZ" to i8*), i8* %1, i32 1)
   %2 = bitcast i8* %.i8 to double*
   %3 = bitcast double* %2 to i8*
-  invoke void (...) @llvm.donothing()
+  invoke void () @llvm.donothing()
           to label %done unwind label %lpad
 
 done:
   ret i8* blockaddress(@"\01?f@@YAXXZ", %try.cont8)
 
 lpad:                                             ; preds = %entry
   %4 = landingpad { i8*, i32 }
           cleanup

 declare i8* @llvm.eh.actions(...) #3
 
 ; Function Attrs: nounwind
 declare void @llvm.frameescape(...) #3
 
 ; Function Attrs: nounwind readnone
 declare i8* @llvm.framerecover(i8*, i8*, i32) #2
 
-declare void @llvm.donothing(...)
+declare void @llvm.donothing()
 
 attributes #0 = { "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-realign-stack" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" "wineh-parent"="␁?f@@YAXXZ" }
 attributes #1 = { "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-realign-stack" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
 attributes #2 = { nounwind readnone }
 attributes #3 = { nounwind }
 attributes #4 = { "wineh-parent"="␁?f@@YAXXZ" }