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

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

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

>


Hi Ayal, thanks for the comments!

> The decision how to vectorize invariant stores also deserves attention: `LoopVectorizationCostModel::setCostBasedWideningDecision()` considers loads from uniform addresses, but not invariant stores - these may end up being scalarized or becoming a scatter; the former is preferred in this case, as the identical scalarized replicas can later be removed.

Yes, the stores are scalarized. Identical replicas left as-is. Either passes such as load elimination can remove it, or we can clean it up in LV itself.

> In any case associated cost estimates should be provided to support overall vectorization costs.

agreed.

> Note that vectorizing **conditional** invariant stores deserves special attention. Unconditional invariant stores are candidates to be sunk out of the loop, preferably before trying to vectorize it.

If we get unconditional invariant stores which haven't been sunk out of the loop and it has reached vectorizer, I think we should let the loop vectorizer vectorize it. Irrespective of what other passes such as LICM should have done with store promotion/sinking. See example in https://bugs.llvm.org/show_bug.cgi?id=38546#c1. Even running through clang++ https://reviews.llvm.org/owners/package/3/ doesn't sink the invariant store out of loop and that store prevents the vectorization of entire loop.

> One approach to vectorize a conditional invariant store is to check if its mask is all false, and if not to perform a single invariant scalar store, for lack of a masked-scalar-store instruction. May be worth distinguishing between uniform and divergent conditions; this check is easier to carry out in the former case.

Thanks, I thought these were automatically handled. Will address in updated patch.



================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:570
   /// else returns false.
   bool hasStoreToLoopInvariantAddress() const {
     return StoreToLoopInvariantAddress;
----------------
Ayal wrote:
> This becomes dead?
The idea is to retain the identification of `storeToLoopInvariantAddress` if other passes which use LAA need it. 
That's the reason I separated out the `StoreToLoopInvariantAddress` and `NonVectorizableStoreToLoopInvariantAddress`.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:578
+    return StoreToLoopInvariantAddress &&
+           NonVectorizableStoreToLoopInvariantAddress;
+  }
----------------
Ayal wrote:
> AND both indicators?
uh oh. was an older change. will fix.


Repository:
  rL LLVM

https://reviews.llvm.org/D50665