[PATCH] D56143: [CallSite removal] Move the verifier to use `CallBase` instead of the `CallSite` wrapper.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Thanks for the review, see comments inline.



================
Comment at: llvm/lib/IR/Verifier.cpp:492
 
-  void verifyCallSite(CallSite CS);
-  void verifySwiftErrorCallSite(CallSite CS, const Value *SwiftErrorVal);
+  void verifyCallBase(CallBase &Call);
+  void verifySwiftErrorCall(CallBase &Call, const Value *SwiftErrorVal);
----------------
compnerd wrote:
> I think it would be nicer to call it `verifyCall`.
I made it `CallBase` to be consistent with `CallInst` and `InvokeInst`, but then I didn't actually make it override the inst visitor method and put it with that list. I've fixed that now. Does this make more sense out of its name?


================
Comment at: llvm/lib/IR/Verifier.cpp:2858
       // Allow sret here and check the wrapped function in verifyStatepoint.
-      if (CS.getCalledFunction() == nullptr ||
-          CS.getCalledFunction()->getIntrinsicID() !=
-            Intrinsic::experimental_gc_statepoint)
+      if (Call.getCalledFunction() == nullptr ||
+          Call.getCalledFunction()->getIntrinsicID() !=
----------------
compnerd wrote:
> I think that this might be easier to read:
> 
> ```
> if (!Call.getCalledFunction() || Call.getCalledFunction()->getIntrinsicID() != Intrinsic:::experimental_gc_statepoint)
> ```
Done, but again, these were mechanical changes. I don't know that I can fix all the bad code that happens to be touched.


================
Comment at: llvm/lib/IR/Verifier.cpp:4129
   // or are local to *this* function.
-  for (Value *V : CS.args())
+  for (Value *V : Call.args())
     if (auto *MD = dyn_cast<MetadataAsValue>(V))
----------------
compnerd wrote:
> Can't this be `const`?
It wasn't before my change, so I'd rather not introduce the potential for still more deltas to code.


================
Comment at: llvm/lib/IR/Verifier.cpp:4394
       // relocates of a call statepoint.
-      auto Token = CS.getArgOperand(0);
+      auto Token = Call.getArgOperand(0);
       Assert(isa<Instruction>(Token) && isStatepoint(cast<Instruction>(Token)),
----------------
compnerd wrote:
> Can't this be `const`?
See above.


================
Comment at: llvm/lib/IR/Verifier.cpp:4418
+    Assert(0 <= DerivedIndex && DerivedIndex < (int)StatepointCall.arg_size(),
+           "gc.relocate: statepoint derived index out of bounds", Call);
 
----------------
compnerd wrote:
> Mind switching to `static_cast<int>`?  `-Wold-style-cast` is .... annoying.
Not really.

We have *so many* of these in the code base that I don't think this is worth doing generally. And moreover, I don't think this is the patch to try and address that.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56143/new/

https://reviews.llvm.org/D56143