[PATCH] D50665: [LV][LAA] Vectorize loop invariant values stored into loop invariant address

[Anna Thomas via Phabricator via llvm-commits]

anna marked 14 inline comments as done.
anna added a comment.

Hi Ayal, thanks for your detailed review!

In https://reviews.llvm.org/D50665#1231754, @Ayal wrote:

> Best allow only a single store to an invariant address for now; until we're sure the last one to store is always identified correctly.


I've updated the patch to restrict to this case for now (diff coming up soon). Generally, if we have multiple stores to an invariant address, it might be canonicalized by InstCombine. So, this may not be as inhibiting as it sounds. Keeping this restriction and allowing "variant stores to invariant addresses" seems like a logical next step once this lands.



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1869
+    HasVariantStoreToLoopInvariantAddress |=
+        (isUniform(Ptr) && !TheLoop->isLoopInvariant(ST->getValueOperand()));
+
----------------
Ayal wrote:
> How about `isUniform(Ptr) && !isUniform(ST->getValueOperand())` ?
> Relying more consistently on SCEV to determine invariance of both address and stored value. Is there a reason for treating stored value more conservatively, checking its invariance by asking if it's outside the loop?
Nothing specific. This works as well. I've changed it. As a separate change, we'll need to improve `isUniform` because they consider uniform FP values are non-uniform (since FP is non-scevable). 


================
Comment at: test/Transforms/LoopVectorize/X86/invariant-store-vectorization.ll:131
+}
+define void @inv_val_load_to_inv_address_conditional(i32* %a, i64 %n, i32* %b, i32 %k) {
+; CHECK-LABEL: @inv_val_load_to_inv_address_conditional(
----------------
Ayal wrote:
> inv_val_load_to?
updated name.


================
Comment at: test/Transforms/LoopVectorize/invariant-store-vectorization.ll:80
+; Both of these tests below are handled as predicated stores and have the cost model
+; as identifying these as predicated stores.
+
----------------
Ayal wrote:
> "as identifying these" >> "identify them"
> Do we check what the cost model identifies?
since we dont have debug statements for what the cost model identifies this, I've updated the above comment.


================
Comment at: test/Transforms/LoopVectorize/invariant-store-vectorization.ll:132
+; For this case, we still vectorize, by generating predicated stores for the if
+; and else cases.
+; TODO: Code gen can be improved by select(extract(vec_cmp(b[i], k), VF - 1) == 1, a = ntrunc, a = k)
----------------
Ayal wrote:
> Hmm, multiple stores to the same invariant address did not trigger LAI memory dependence checks(?)
> This may generate wrong code if the conditional scalarized stores are emitted in the wrong order, or if a pair of masked scatters are used.
good point - as stated in comment earlier, I will restrict to one store to invariant address for now.


================
Comment at: test/Transforms/LoopVectorize/invariant-store-vectorization.ll:219
+; is the latch block (which doesn't need to be predicated).
+; TODO: We should vectorize this loop.
+; CHECK-LABEL: inv_val_store_to_inv_address_conditional_inv
----------------
Ayal wrote:
> .. efficiently once divergence analysis identifies storeval as uniform
once we relax the check of variant/invariant value being stored, it does not matter if we correctly identify if it is variant or invariant. So, I think divergence analysis is not required. 


Repository:
  rL LLVM

https://reviews.llvm.org/D50665