[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:250
+  if (CallInst *CI = dyn_cast<CallInst>(&I)) {
+    for (Value *Op : CI->arg_operands())
+      if (Op->getType()->isPointerTy()) {
----------------
bmahjour wrote:
> RithikSharma wrote:
> > bmahjour wrote:
> > > I think the call itself should also go into the MemLocs vector.
> > CallInst returns none memory location, which won't be useful to find dependence info and we may have to skip it so I did not add it into the MemLocs in the first place. 
> Then we have a problem if the memory accesses of a function are not represented through their arguments. For example consider:
> ```
> int a = 0;
> 
> void foo() {
>   a = 1;
> }
> 
> int main() {
>   foo();
>   a = 3;
>   return a;
> }
> ```
> In the case above `a = 3` should not be allowed to move before the call to `foo()`.
> 
> If this information is not available through AliasAnalysis, then we have to check for it separately. One way to solve it is to look at the underlying object referenced by `I` and `Inst`, and if any of them is a global and one of the intervening instructions is a call that may have side-effect, then return false. 
Please add this test case, and update the patch.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:298
+    return isDependenceSafe(I, *DI, InstsToCheck);
+  else if (AA)
+    return isDependenceSafe(I, *AA, InstsToCheck);
----------------
hiraditya wrote:
> `else` is not needed,
that's true, please remove the `else`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84589