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

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:248
+
+  SmallVector<MemoryLocation, 10> MemLocs;
+  if (CallInst *CI = dyn_cast<CallInst>(&I)) {
----------------
Any reason we have '10' instead of a value which is power of 2?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:260
+  for (MemoryLocation MemLoc : MemLocs) {
+    IsSafe &=
+        llvm::none_of(InstsToCheck, [&AA, &I, &MemLoc](Instruction *Inst) {
----------------
Once `IsSafe` is false. should we just return and avoid subsequent checks?


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:298
+    return isDependenceSafe(I, *DI, InstsToCheck);
+  else if (AA)
+    return isDependenceSafe(I, *AA, InstsToCheck);
----------------
`else` is not needed,


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:387
+                              DependenceInfo *DI, AAResults *AA) {
   // Skip tests when we don't have PDT or DI
+  if (!PDT || !(DI || AA))
----------------
comment needs update.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:456
                               DominatorTree &DT, const PostDominatorTree *PDT,
-                              DependenceInfo *DI) {
+                              DependenceInfo *DI, AAResults *AA) {
   return llvm::all_of(BB, [&](Instruction &I) {
----------------
Are we planning to make use of `AA` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84589