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

[Igor Kirillov via Phabricator via llvm-commits]

igor.kirillov added a comment.

> 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.

Is it worth for me to explore if it possible to do other way around or we should work on with the solution from this merge?



================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:642
+  /// List of stores to uniform addresses.
+  SmallVector<StoreInst *, 5> InvariantStores;
+
----------------
david-arm wrote:
> fhahn wrote:
> > Are the stores invariant or to a uniform address? `InvariantStores` implies they are invariant, which may not be the case?
> I assume here that InvariantStores refers to stores to an invariant (i.e. uniform?) address, rather than storing an invariant value to variant address. If so, perhaps it could be named StoresToInvariantAddress or something like that?
@fhah Just to make sure we are on the same line - for me uniform and invariant are just synonyms, isn't it so?


================
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);
+
----------------
fhahn wrote:
> Is `LoopUtils.h` the right place for this? Is there anything loop related?
What do you think if I make this function a public member of `class ScalarEvolution`? Or is there a better place for it?


================
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).
----------------
fhahn wrote:
> 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?
I checked and only `LoopInterchangeLegality` is using this function and it is not affected by stores. Anyway, I can add a parameter to `RecurrenceDescriptor::isReductionPHI` or a member to `RecurrenceDescriptor` allowing or not to handle stores. What do you think about it?

The comment is to be updated, yes.


================
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)) {
----------------
fhahn wrote:
> 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?
Loads from or stores to the same address in the loop? I'm sorry could you clarify what the problem is. As it is I don't understand the message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235