[PATCH] D62607: LoopDistribute/LAA: Respect convergent

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1797
+        if (Call->isConvergent())
+          HasConvergentOp = true;
+      }
----------------
What is the reason to not set `CanInsertRuntimeCheck = false` directly?

If it's because of the early exist in the following loop, since we are no computing two orthogonal properties instead of one, shouldn't we ensure that that we iterate over all instructions. That is, we may early-exit because it is not vectorizable, but it might still be distributable if there are no convergent instructions.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1977
   LLVM_DEBUG(
-      dbgs() << "LAA: We can perform a memory runtime check if needed.\n");
+    dbgs() << "LAA: May be able to perform a memory runtime check if needed.\n");
 
----------------
This message carries no information. Remove entirely/move the check from line 2012 before this? The entire code after this seems useless when a convergent call has been found anyway.


================
Comment at: lib/Transforms/Scalar/LoopDistribute.cpp:694
+    if (!LAI->canInsertRuntimeCheck() &&
+        LAI->getNumRuntimePointerChecks() != 0) {
+      return fail("RuntimeCheckWithConvergent",
----------------
[serious] Isn't `PredicatedScalarEvolution` another source of runtime checks? We should ensure that it's predicate is always true. That is, LoopDistribute's condition is `!Pred.isAlwaysTrue() || !Checks.empty()`.


================
Comment at: lib/Transforms/Scalar/LoopDistribute.cpp:695-696
+        LAI->getNumRuntimePointerChecks() != 0) {
+      return fail("RuntimeCheckWithConvergent",
+                  "may not insert runtime check with convergent operation");
+    }
----------------
The error message does not match the API `canInsertRuntimeCheck`. We might add more reasons why we cannot have a runtime check. If the `analyzeLoop` exists early before reaching `CanInsertRuntimeCheck = !HasConvergentOp`, there might not even be a convergent operation in the loop.


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

https://reviews.llvm.org/D62607