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

[Anna Thomas via Phabricator via llvm-commits]

anna marked an inline comment as done.
anna added inline comments.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:570
   /// else returns false.
   bool hasStoreToLoopInvariantAddress() const {
     return StoreToLoopInvariantAddress;
----------------
anna wrote:
> Ayal wrote:
> > 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.
> yes, that's right. I made the change, but the analysis has an ORE and there are 5 tests in the LoopAccessAnalysis that are failing because the ORE check "Store to invariant address was [not] found in loop" is missing. See test/Analysis/LoopAccessAnalysis/store-to-invariant-check1.ll  where it looks for the presence of "Store to invariant address was found in loop". 
> 
> I'll remove the code as a follow on clean up and if there's a need for this by other passes that use LAA, folks can add it back when required.
> 
> I think it also makes sense to add an ORE for the "VariantStoreToInvariantAddress" as part of this current change.
> 
I've made both the changes in this patch since changing the ORE is clearer in one patch.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5880
         unsigned Cost = getUniformMemOpCost(&I, VF);
         setWideningDecision(&I, VF, CM_Scalarize, Cost);
         continue;
----------------
Ayal wrote:
> 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.
thanks for bringing this up. It exercised the `X86TTIImpl::getMemoryOpCost` which showed the bug in my previous diff for `LoopVectorizationCostModel::getUniformMemOpCost` for uniform store. I was passing in the store's type instead of the store val type. 
I've also updated it to use the "unified" interface for load/store just like the other cost model calculations - `getGatherScatterCost` etc.


Repository:
  rL LLVM

https://reviews.llvm.org/D50665