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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In https://reviews.llvm.org/D50665#1200509, @anna wrote:

>


...

> 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.



- - by revisiting LoopVectorizationCostModel::collectLoopUniforms()?  ;-)



================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:570
   /// else returns false.
   bool hasStoreToLoopInvariantAddress() const {
     return StoreToLoopInvariantAddress;
----------------
anna wrote:
> 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`.
OK. But LoopVectorizationLegality below seems to be its only user.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:638
+  /// StoreToLoopInvariantAddress.
+  bool NonVectorizableStoreToLoopInvariantAddress;
   /// The diagnostics report generated for the analysis.  E.g. why we
----------------
Better name it more accurately as, e.g., `VariantStoreToLoopInvariantAddress`?


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1865
 
+  auto isLoopInvariant = [this](StoreInst *ST) {
+    auto StoreVal = ST->getValueOperand();
----------------
`isLoopInvariantStoreValue` ?


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1871
+      return false;
+    return TheLoop->hasLoopInvariantOperands(cast<Instruction>(StoreVal));
+  };
----------------
Again, something LICM may have missed?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5754
                                                          unsigned VF) {
+  if (isa<LoadInst>(I)) {
   LoadInst *LI = cast<LoadInst>(I);
----------------
Can use `if (auto *LI = dyn_cast<LoadInst>(I)) {`


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5763
          TTI.getMemoryOpCost(Instruction::Load, ValTy, Alignment, AS) +
          TTI.getShuffleCost(TargetTransformInfo::SK_Broadcast, VectorTy);
+  }
----------------
Indent


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5880
         unsigned Cost = getUniformMemOpCost(&I, VF);
         setWideningDecision(&I, VF, CM_Scalarize, Cost);
         continue;
----------------
On certain targets, e.g., skx, an invariant store may end up as a scatter, so setting this decision here to avoid that is important; potentially worthy of a note / a test.


Repository:
  rL LLVM

https://reviews.llvm.org/D50665