[PATCH] D55641: [CallSite removal] Migrate all Alias Analysis APIs to use the newly minted `CallBase` class instead of the `CallSite` wrapper.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 7 inline comments as done.
chandlerc added a comment.

Sorry for arguing on these...



================
Comment at: llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h:400
   /// that.
-  const NonLocalDepInfo &getNonLocalCallDependency(CallSite QueryCS);
+  const NonLocalDepInfo &getNonLocalCallDependency(CallBase *QueryCall);
 
----------------
compnerd wrote:
> Hmm, I realize that the `QueryCall` is used for an index via pointer, but, could we not take this by reference?  This shouldn't be null ever, so the reference would be nicer.
That would be inconsistent with the majority of this API. I personally prefer the style you are suggesting here, but I don't want to make *only* these parameters differ, and I don't want to try to move the entire API to references in this patch, so I think these should stay pointers for now.


================
Comment at: llvm/include/llvm/Analysis/MemoryLocation.h:234
   /// Return a location representing a particular argument of a call.
-  static MemoryLocation getForArgument(ImmutableCallSite CS, unsigned ArgIdx,
+  static MemoryLocation getForArgument(const CallBase *Call, unsigned ArgIdx,
                                        const TargetLibraryInfo *TLI);
----------------
compnerd wrote:
> Similar.
Same as above, see the rest of this API which takes pointers.


================
Comment at: llvm/lib/Analysis/AliasSetTracker.cpp:472
+
+      for (auto IdxArgPair : enumerate(Call->args())) {
+        int ArgIdx = IdxArgPair.index();
----------------
compnerd wrote:
> I wish that we could use something like `std::tie` to unpack in the loop declaration as in C++17.
I mean, me too.


================
Comment at: llvm/lib/Analysis/AliasSetTracker.cpp:474
+        int ArgIdx = IdxArgPair.index();
+        const Value *Arg = IdxArgPair.value();
+        if (!Arg->getType()->isPointerTy())
----------------
compnerd wrote:
> I think that using `std::tie` to unpack is nicer.
I disagree once it has to be here.

It forces us to use 3 lines instead of 2, and to leave variables uninitialized. And the only duplicated code here is `IdxArgPair`. Plus we have useful methods here. I think, given the choices, `std::tie` is not an improvement (by a narrow margin), even though structured bindings when we have them will be a huge improvement.


================
Comment at: llvm/lib/Analysis/CaptureTracking.cpp:275
+        int Idx = IdxOpPair.index();
+        Value *A = IdxOpPair.value();
+        if (A == V && !Call->doesNotCapture(Idx))
----------------
compnerd wrote:
> I think that `std::tie` is nicer.
As above. I'll keep them in sync.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:160
       // ignore calls that only access local memory.
-      for (CallSite::arg_iterator CI = CS.arg_begin(), CE = CS.arg_end();
+      for (CallSite::arg_iterator CI = Call->arg_begin(), CE = Call->arg_end();
            CI != CE; ++CI) {
----------------
compnerd wrote:
> Why not convert this to a range based for loop:?
I was trying to make minimal changes as this is already a massive patch.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:440
     const MemoryDependenceResults::NonLocalDepInfo &deps =
-      MD->getNonLocalCallDependency(CallSite(C));
+        MD->getNonLocalCallDependency(C);
     // FIXME: Move the checking logic to MemDep!
----------------
compnerd wrote:
> Might be nice to make that down-cast to a `CallBase` explicit.
That is exceedingly rare in LLVM code, and I don't know why we would o it here really... It doesn't seem to add any important information?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55641