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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

> to SSA form due to lack of aliasing info

It would be good to mention *why* those particular stores can be sunk during loop vectorization. IIUC the patch relies on the runtime checks generated to ensure the store does not alias other accesses in the loop.

It seems like the main problem is that we potentially bail out too early at the moment when checking for reductions due to the store, but once we generate runtime checks, sinking the store may become legal (see inline comment about loads to the same address)? If that's the case, ideally we'd just sink any such loads/stores before detecting reductions once we know they can be sunk due to runtime checks, but unfortunately I do not think that's possible with the current structure/ordering.



================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:642
+  /// List of stores to uniform addresses.
+  SmallVector<StoreInst *, 5> InvariantStores;
+
----------------
Are the stores invariant or to a uniform address? `InvariantStores` implies they are invariant, which may not be the case?


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:402
+/// Returns true if A and B have same pointer operands or same SCEVs addresses
+bool storeToSameAddress(ScalarEvolution *SE, StoreInst *A, StoreInst *B);
+
----------------
Is `LoopUtils.h` the right place for this? Is there anything loop related?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:301
+  //  - By store instructions with a loop invariant address (safe).
+  //      * If there are several stores, all must have the same address.
   //  - By an instruction that is not part of the reduction (not safe).
----------------
What about existing users of `isReductionPHI` which currently may rely on the fact that all instruction in the loop must either be phis or reduction operations? Also, with respect to the store restriction, the important bit is that the final value is also stored, right?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:310
+    // Store instructions are allowed iff it is the store of the reduction
+    // value to the same loop uniform memory location.
+    if (auto *SI = dyn_cast<StoreInst>(Cur)) {
----------------
You are only checking for loop-invariant addresses, so should this be `loop invariant memory location`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:900
+    // of one of our reductions and is unconditional.
+    for (StoreInst *SI : LAI->getInvariantStores()) {
+      if (isRecurringInvariantStore(SI, &IsPredicated)) {
----------------
What about loads to the same address in the loop? At the moment, `LAA` cannot analyze dependences with invariant addresses. But if this limitation gets removed the code here may become incorrect, because it relies on this limitation IIUC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235