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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the update with the tests! I think there might still be a correctness issue. More details inline.



================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:178
+  /// computed. If \p SE is non-null, store instructions to loop invariant
+  /// address are processed.
+  static bool
----------------
nit: addresses ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:963
+        if (isRecurringInvariantStore(SI)) {
+          // Earlier stores to this address are effectively deadcode.
+          erase_if(UnhandledStores, [SE, SI](StoreInst *I) {
----------------
I'm not sure if the comment matches the code. There's no guarantee that all stores to the same address come before the store of the reduction AFAICT. E.g. the test below has a store to the same address after the store of the reduction. I think this may get mis-compiled, as the store to 0 gets replaced with the store of the final value of the reduction.

```
define void @reduc_store(i32* %dst, i32* readonly %src) {
entry:
  %gep.dst = getelementptr inbounds i32, i32* %dst, i64 42
  store i32 1, i32* %gep.dst, align 4
  br label %for.body

for.body:
  %sum = phi i32 [ 0, %entry ], [ %add, %for.body ]
  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
  %gep.src = getelementptr inbounds i32, i32* %src, i64 %iv
  %0 = load i32, i32* %gep.src, align 4
  %add = add nsw i32 %sum, %0
  store i32 %add, i32* %gep.dst, align 4
  store i32 0, i32* %gep.dst, align 4
  %iv.next = add nuw nsw i64 %iv, 1
  %exitcond = icmp eq i64 %iv.next, 1000
  br i1 %exitcond, label %exit, label %for.body

exit:
  ret void
}
```


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1020
+    StoreInst *DSI = DS.IntermediateStore;
+    if (DSI && DSI == SI)
+      return true;
----------------
nit: can drop `DSI`, as `SI` is guaranteed to be non-null?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235