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

[Igor Kirillov via Phabricator via llvm-commits]

igor.kirillov marked 7 inline comments as done.
igor.kirillov added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:21
 #include "llvm/ADT/StringRef.h"
+#include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstrTypes.h"
----------------
peterwaller-arm wrote:
> 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
Fixed


================
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()) {
----------------
peterwaller-arm wrote:
> Does this comment need updating to discuss your intermediate stores?
Yes, indeed


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:458
+              NonPHIs.push_back(UI);
+          } else
+            NonPHIs.push_back(UI);
----------------
peterwaller-arm wrote:
> 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).
I hope this one clarifies. And, actually we should exit if reduction variable is used as an address (this should be highly unlikely case but nevertheless)


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction.ll:539
+;    sum += src[i+1];
+;    dst[42] = sum;
+;  }
----------------
peterwaller-arm wrote:
> Suggestion for a test: what about testing two different destination addresses modified in the loop body?
Actually this is not supported so far, only one invariant address is possible. (See llvm/lib/Analysis/IVDescriptors.cpp:318).  Do you think we need test that checks that vectorisation is not happening?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235