[PATCH] D110235: [LoopVectorize] Support reductions that store intermediary result

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.

LGTM, thanks! I added a few additional comments that can be addressed directly before committing.

FYI work has started to model the exit block in VPlan as well: D123457 <https://reviews.llvm.org/D123457>, D123537 <https://reviews.llvm.org/D123537>. `fixReduction` and the store sinking can and should now be migrated to be modeled explicitly in VPlan.



================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:273
 
+  /// Intermediate store of the reduction
+  StoreInst *IntermediateStore = nullptr;
----------------
nit: would be good to also document what this means, what the properties of such stores are.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:310
 
+  /// Returns True if given invariant store uses reccurent expression
+  /// \p IsPredicated shows if block containing \p SI needs predication
----------------
peterwaller-arm wrote:
> nit. 'recurrent'
nit: could be more specific, the increment of the reduction is  used as stored operand, right?

Also, they only handle reductions right? Then using Reduction instead of Recurrence seems more descriptive, .e.g. `isInvariantStoreOfReduction`? (same for `isRecurringInvariantAddress`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:453
+  Value *APtr = A->getPointerOperand();
+  Value *BPtr = B->getPointerOperand();
+  if (APtr == BPtr)
----------------
With opaque pointers, this code may behave differently.

It's possible to have

```
store i32 0, ptr %x
store i8 0, ptr %x
```

In that case the pointer operands will be the same, but different store widths. I *think* we should also check that the types of the stored values match for now, as we use this to remove earlier stores. This should only be correct if the later store writes at least as many bits as the earlier stores.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1020
+bool LoopVectorizationLegality::isRecurringInvariantStore(StoreInst *SI) {
+  for (auto &Reduction : Reductions) {
+    RecurrenceDescriptor &DS = Reduction.second;
----------------
nit: could be any_of


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1030
+  ScalarEvolution *SE = PSE.getSE();
+  for (auto &Reduction : Reductions) {
+    RecurrenceDescriptor &DS = Reduction.second;
----------------
nit: could be any_of.


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-with-invariant-store.ll:412
+;
+; for(int i=0; i < 1000; i++) {
+;   sum += src[i];
----------------
Would it be possible to add runtime tests for the mis-compiles fixed to https://github.com/llvm/llvm-test-suite/tree/main/SingleSource/UnitTests/Vectorizer?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110235/new/

https://reviews.llvm.org/D110235