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

[rithik sharma via Phabricator via llvm-commits]

RithikSharma added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:292
+          }
+          if (MemLoc.Size.hasValue() && DestMemLoc.Size.hasValue() &&
+              AA.isNoAlias(MemLoc, DestMemLoc))
----------------
Whitney wrote:
> This is no longer needed.
Thanks, I'll include this in the next update.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:280
+            DestResult = AA.getModRefInfo(&I, DestMemLoc);
+            if (AA.isNoAlias(MemLoc, DestMemLoc))
+              return false;
----------------
Whitney wrote:
> Whitney wrote:
> > 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`.
> I am confused why cannot change to the code as suggested above. 
> 
> This current code seems to have some code duplication.
I'll verify that and look more what's missing, extra and not in sequence. I will update the patch with required changes asap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84589