[PATCH] D54538: [LV] Avoid vectorizing unsafe dependencies in uniform address

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:567
 
-  /// If the loop has multiple stores to an invariant address, then
+  /// If the loop has non-vectorizable stores to an invariant address, then
   /// return true, else return false.
----------------
Comment is pretty obvious; better explain when stores to an invariant address are considered non-vectorizable.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1878
+      HasNonVectorizableStoresToLoopInvariantAddress |=
           !UniformStores.insert(Ptr).second;
+      UniformStoreMap[Ptr] = ST;
----------------
Is `UniformStores` still needed? Can check instead `UniformStoreMap.count(Ptr)` here (and below)?


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1926
+    if (!HasNonVectorizableStoresToLoopInvariantAddress &&
+        UniformStores.find(Ptr) != UniformStores.end() &&
+        !DT->dominates(UniformStoreMap[Ptr], LD)) {
----------------
Can check instead `UniformStoreMap.count(Ptr)`?

Note that checking `!HasNonVectorizableStoresToLoopInvariantAddress` is redundant, but could save time.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1927
+        UniformStores.find(Ptr) != UniformStores.end() &&
+        !DT->dominates(UniformStoreMap[Ptr], LD)) {
+      LLVM_DEBUG(dbgs() << "LAA: Found an unsafe dependency between a uniform "
----------------
efriedma wrote:
> I'm not sure the dominance check is sufficient. Yes, if the store dominates the load, it's theoretically possible to vectorize the loop, but only if vectorizer has a special case to forward the stored values to the load.  Otherwise, the load will get values from the wrong loop iteration.
Ahh, right! No special forwarding provided. Only the last of the VF stores to same address will survive, and it will feed all VF loads.

As @anna originally intended: if an invariant address is both stored-to and loaded-from, inside the loop, bail out.

So can continue to use the `ValueSet UniformStores`;


================
Comment at: test/Transforms/LoopVectorize/invariant-store-vectorization.ll:555
+; to the same address
+define void @unsafe_dep_uniform_load_store(i32 %arg, i32 %arg1, i64 %arg2, i16* %arg3, i32 %arg4, i64 %arg5) {
+; CHECK-LABEL: unsafe_dep_uniform_load_store
----------------
Good to mention this test is related to pr39653.

With -force-vector-width=4 the test can be simplified; it probably took the additional instructions to convince LV's cost-model to vectorize the original loop on its own, w/o -force-vector-width.


Repository:
  rL LLVM

https://reviews.llvm.org/D54538