[PATCH] D84589: [CodeMoverUtils] Add optional data dependence checks using Alias Analysis

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:280
+            DestResult = AA.getModRefInfo(&I, DestMemLoc);
+            if (AA.isNoAlias(MemLoc, DestMemLoc))
+              return false;
----------------
Whitney wrote:
> Is it safe to check this first if the memory locations are not None?
> Something like...
> ```
>           auto DestMemLoc = MemoryLocation::get(Inst);
>           if (MemLoc.hasValue() && DestMemLoc.hasValue() &&  AA.isNoAlias(MemLoc, DestMemLoc))
>               return false;
> 
>           ModRefInfo Result = AA.getModRefInfo(Inst, MemLoc);
>           ModRefInfo DestResult = AA.getModRefInfo(&I, DestMemLoc);
>           if (CallBase *CBDest = dyn_cast<CallBase>(Inst)) {
>             if (!MemLoc.hasValue())
>               Result = DestResult = AA.getModRefInfo(&I, CBDest);
>           if (CallBase *CBSrc = dyn_cast<CallBase>(&I))
>             if (!DestMemLoc.hasValue())
>               DestResult = AA.getModRefInfo(Inst, CBSrc);
> ```
I mean check `isNoAlias` first, before querying `getModRefInfo`.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:252
+      return false;
+    auto DestMemLoc = MemoryLocation::get(Inst);
+
----------------
RithikSharma wrote:
> Whitney wrote:
> > RithikSharma wrote:
> > > bmahjour wrote:
> > > > MemoryLocation::get returns "none" if Inst is a call. Please add special handling for calls and make sure your tests cover it.
> > > Test case pending.
> > Is the test cases for call going to be added in this patch?
> Test cases are present in https://reviews.llvm.org/D84776
Can you move specifically the part that test call instructions here? And the test in D84776 doesn't have any call instructions with pointer operands, so this code is not tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84589