[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;
----------------
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);
```


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:433
   // never return, from I to InsertPoint.
-  if (!isSafeToSpeculativelyExecute(&I))
-    if (std::any_of(InstsToCheck.begin(), InstsToCheck.end(),
-                    [](Instruction *I) {
-                      if (I->mayThrow())
-                        return true;
-
-                      const CallBase *CB = dyn_cast<CallBase>(I);
-                      if (!CB)
-                        return false;
-                      if (!CB->hasFnAttr(Attribute::WillReturn))
-                        return true;
-                      if (!CB->hasFnAttr(Attribute::NoSync))
-                        return true;
-
-                      return false;
-                    })) {
-      return reportInvalidCandidate(I, MayThrowException);
-    }
+  if (std::any_of(InstsToCheck.begin(), InstsToCheck.end(), [](Instruction *I) {
+        if (I->mayThrow())
----------------
why remove `isSafeToSpeculativelyExecute`?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:260
+  for (MemoryLocation MemLoc : MemLocs) {
+    IsSafe &=
+        llvm::none_of(InstsToCheck, [&AA, &I, &MemLoc](Instruction *Inst) {
----------------
hiraditya wrote:
> Once `IsSafe` is false. should we just return and avoid subsequent checks?
You don't need to variable `IsSafe` anymore, as you return false right away if `IsSafe` is false. And you can return true at the end of the function instead.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:264
+
+          auto DestMemLoc = MemoryLocation::get(Inst);
+
----------------
bmahjour wrote:
> `Inst` (from the list of instructions being checked) may also be a call.
> // This can be none for CallInst but this won't be used for CallInst

I don't understand this comment. 


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:252
+      return false;
+    auto DestMemLoc = MemoryLocation::get(Inst);
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84589