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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

LGTM, with minor optional comments. Would be good to add a test where the load is preceded by a store in the loop, indicating that such dependence is also non-vectorizable.



================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:569
+  /// dependency with a uniform load, then return true, else return false.
+  bool hasNonVectorizableStoresToLoopInvariantAddress() const {
+    return HasNonVectorizableStoresToLoopInvariantAddress;
----------------
Can simply state that if there's a memory dependence involving an invariant address, i.e., two stores or a store and a load, return true, else return false.

Could alternatively name it `hasDependenceInvolvingLoopInvariantAddress`

Note that a load from an invariant address that depends on store(s) to it in the loop, should ideally be promoted to use the temporary being stored; the temporary value gets expanded, unlike the store to the invariant address. The missed ORE message could indicate such potential resolution.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1919
+    // store to the same uniform address.
+    if (UniformStores.find(Ptr) != UniformStores.end()) {
+      LLVM_DEBUG(dbgs() << "LAA: Found an unsafe dependency between a uniform "
----------------
Can alternatively use `if (UniformStores.count(Ptr))`


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:819
     return false;
+  }
 
----------------
No need for {}


================
Comment at: test/Transforms/LoopVectorize/invariant-store-vectorization.ll:568
+  %tmp9 = phi i32 [ %arg1, %bb ], [ %tmp23, %bb7 ]
+  %tmp10 = load i32, i32* %tmp
+  %tmp11 = mul nsw i32 %tmp9, %tmp10
----------------
(Note: load could be replaced by phi(%arg4, %tmp12), a potentially vectorizable 1st-order-recurrence.)


Repository:
  rL LLVM

https://reviews.llvm.org/D54538