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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Thanks for taking care of everything, this LGTM now, added only a few minor optional comments.



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1883
+      HasVariantStoreToLoopInvariantAddress = true;
+
     // If we did *not* see this pointer before, insert it to  the read-write
----------------
Maybe clearer to do

```
  if (isUniform(Ptr)) {
    // Consider multiple stores to the same uniform address as a store of a variant value.
    bool MultipleStoresToUniformPtr = UniformStores.insert(Ptr).second;
    HasVariantStoreToLoopInvariantAddress |= (!isUniform(ST->getValueOperand()) || MultipleStoresToUniformPtr);
  }
```

Note that supporting a single store of a variant value to an invariant address is easier than supporting multiple (conditional) stores of invariant values to an invariant address, as discussed. So the two conditions should probably be separated when the patch taking care of the former is introduced.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1180
+  /// Store: scalar store + (loop invariant value stored? 0 : extract of last
+  /// element)
   unsigned getUniformMemOpCost(Instruction *I, unsigned VF);
----------------
The ": extract of last element" part is for future use, when stores of variant values to invariant addresses are supported, right? Best leave this part to that future patch, or add a TODO to test this extra cost then.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5422
         NumPredStores++;
+      }
 
----------------
No need to add these enclosing curly brackets.


================
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
----------------
anna wrote:
> 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. 
ok, works both ways - once we leverage divergence analysis we'll be able to handle such a store of uniform/invariant value, w/o needing relaxed support for stores of variant values.


Repository:
  rL LLVM

https://reviews.llvm.org/D50665