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

[Anna Thomas via Phabricator via llvm-commits]

anna marked 4 inline comments as done.
anna added inline comments.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:570
   /// else returns false.
   bool hasStoreToLoopInvariantAddress() const {
     return StoreToLoopInvariantAddress;
----------------
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.



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

JFI- Today the changed name (VariantStoreToLoopInvariantAddress)  is accurate. However, my plan is to eventually teach the vectorizer about all *safe* uniform stores, not just invariant values stored to invariant address.

So `VariantStoreToLoopInvariantAddress` can also be vectorized under certain conditions (safe dependence distance calculation for the store versus other memory access). So something like example below can be vectorized [1]:
```
  for (i=0; i<n;i++)
      for (j=0; j<n; j++) {
             p[i] = b[j]; 
             z[j] += b[j];
      }   
}
```

However, this cannot be vectorized safely:
```
  for (i=0; i<n;i++)
      for (j=0; j<n; j++) {
             z[j] = (++p[i]);  <-- dependence distance for uniform store and load is 1.
      }   
}
```
[1] LICM should try to sink the store out of inner loop, but sometimes it cannot do so because it cannot prove dereferencability for the store address or that the store is guaranteed to execute at least once.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1871
+      return false;
+    return TheLoop->hasLoopInvariantOperands(cast<Instruction>(StoreVal));
+  };
----------------
Ayal wrote:
> Again, something LICM may have missed?
yes, LICM misses this as well - see added test case in `inv_val_store_to_inv_address_conditional_inv`. 


Repository:
  rL LLVM

https://reviews.llvm.org/D50665