[PATCH] D62607: LoopDistribute/LAA: Respect convergent

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur requested changes to this revision.
Meinersbur added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1799
+          HasConvergentOp = true;
+          if (HasComplexMemInst)
+            return;
----------------
Could you add a small comment like `With both, a non-vectorizable memory instruction and a convergent operation, found in this loop, no reason to continue the search`?

You could alternatively add a `if (HasComplexMemInst && HasConvergentOp) break` after this check to break searching as soon as both conditions are true, independently of which one is found first.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1805
+      // Keep looking for convergent calls even after vectorization is known to
+      // ben impossible.
+      if (HasComplexMemInst)
----------------
[typo] `ben`


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1877
     LLVM_DEBUG(dbgs() << "LAA: Found a read-only loop!\n");
     CanVecMem = true;
     return;
----------------
[serious] Is a `if (!CanVecMem) return;` missing before this? This would switch it back on again independently of whether `HasComplexMemInst` is set.


================
Comment at: lib/Transforms/Scalar/LoopDistribute.cpp:805
+    if (LAI->hasConvergentOp() && !Checks.empty()) {
+      return fail("RuntimeCheckWithConvergent",
+                  "may not insert runtime check with convergent operation");
----------------
Is there a reason to not check `Pred.isAlwaysTrue()` and `!Checks.empty()` in the same conditional?


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

https://reviews.llvm.org/D62607