[PATCH] D62607: LoopDistribute/LAA: Respect convergent

[Kit Barton via Phabricator via llvm-commits]

kbarton added inline comments.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1795
     for (Instruction &I : *BB) {
+      if (auto *Call = dyn_cast<CallBase>(&I)) {
+        if (Call->isConvergent())
----------------
I don't think the braces are necessary here. 


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1797
+        if (Call->isConvergent())
+          HasConvergentOp = true;
+      }
----------------
Meinersbur wrote:
> 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.
I think I agree with this. If I understand things correctly, it's probably better to look for the convergent op separately from this loop, and ensure we iterate over all the instructions in the loop since we're now looking for two, conceptually different, things. 


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:2017
+                         "would be needed with a convergent operation\n");
+    CanVecMem = false;
+    return;
----------------
Maybe I'm missing something here, but shouldn't this case (HasConvergentOp) go in the condition above (isDependencyCheckNeeded)?
In other words, is it not possible that we do not need a runtime check and thus can still vectorize?


================
Comment at: test/Analysis/LoopAccessAnalysis/unsafe-and-rt-checks-convergent.ll:9
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.10.0"
+
----------------
I don't think the triple is needed.


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

https://reviews.llvm.org/D62607