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

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

I'm in the process of increasing my knowledge of this kind of thing, so not an authoritative review from me. Have passed it through my eyeballs, hopefully can add some comments for useful general improvements.



================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:21
 #include "llvm/ADT/StringRef.h"
+#include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstrTypes.h"
----------------
igor.kirillov wrote:
> nikic wrote:
> > Drive by note: This new include does not look necessary.
> Unfortunately, llvm doesn't compile without this include 
It looks like the needed include is #include "llvm/IR/Instructions.h" for StoreInst, or better forward declare `class StoreInst;` along with the others nearby.

https://include-what-you-use.org
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md


================
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
----------------
nit. 'recurrent'


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:306
   //      * An instruction type other than PHI or the reduction operation.
   //      * A PHI in the header other than the initial PHI.
   while (!Worklist.empty()) {
----------------
Does this comment need updating to discuss your intermediate stores?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:458
+              NonPHIs.push_back(UI);
+          } else
+            NonPHIs.push_back(UI);
----------------
Suggestion: Does this warrant a comment? (I only observe that there are lots of comments around here and I spent a moment trying to guess at what this did without arriving at an answer).


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1992
+      // Consider multiple stores to the same uniform address as a store of a
+      // variant value.
+      InvariantStores.push_back(ST);
----------------
Does the word 'variant' add anything? I couldn't find any other uses nearby which elucidate what you're trying to say here. It feels confusing because you are talking about 'InvariantStores' which means invariant in the address, so it looks like a typo. I feel this comment could be better: 'Record stores instructions to loop-invariant addresses', in contrast to the comment on UniformStores above? Do you need to say much about the value?




================
Comment at: llvm/test/Transforms/LoopVectorize/reduction.ll:539
+;    sum += src[i+1];
+;    dst[42] = sum;
+;  }
----------------
Suggestion for a test: what about testing two different destination addresses modified in the loop body?


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction.ll:590
+
+; CHECK-: vector.body:
+; CHECK-NOT: store i32 %{{[0-9]+}}, i32* %arrayidx
----------------
CHECK-?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235