[PATCH] D25517: [SLPVectorizer] Improved support of partial tree vectorization.

[Adam Nemet via Phabricator via llvm-commits]

anemet added a comment.

Please write a new patch with all these improvements and add me as a reviewer.  Thanks.



================
Comment at: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp:4457
+namespace {
+/// Tracks instructons and its children.
+class WeakVHWithLevel final : public CallbackVH {
----------------
ABataev wrote:
> anemet wrote:
> > Tracks for what?
> Traks if the instruction is deleted (replaced by undef value) /replaced by the new instruction (the vector version, as the result of the whole vectorization process) + tracks the processing of the instruction operands
Improve the comment then, please.


================
Comment at: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp:4460
+  /// Operand index of the instruction currently beeing analized.
+  unsigned Level = 0;
+  /// Is this the instruction that should be vectorized, or are we now
----------------
OperandIndexAnalyzed?


================
Comment at: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp:4464
+  /// vectorization?
+  bool IsInitial = true;
+
----------------
IsAnalyzingOperands?

Or just merge these into a single state variable, Optional<unsigned> which if None means we're processing the instruction itself.


================
Comment at: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp:4499-4505
 /// \brief Attempt to reduce a horizontal reduction.
 /// If it is legal to match a horizontal reduction feeding
-/// the phi node P with reduction operators BI, then check if it
-/// can be done.
+/// the phi node P with reduction operators Root in a basic block BB, then check
+/// if it can be done.
 /// \returns true if a horizontal reduction was matched and reduced.
 /// \returns false if a horizontal reduction was not matched.
+static bool canBeVectorized(
----------------
ABataev wrote:
> anemet wrote:
> > The name of the function and the comment mismatch.  What is this function supposed to do?
> Yes, probably. This function checks if it is possible to vectorize the tree + performs the vectorization of horizontal reduction or, if the instruction is not the top instruction of the horizontal reduction and this is a binary operation, vectorizes the operands of this binary instruction.
Then improve the comment please.


================
Comment at: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp:4517-4573
+  SmallVector<WeakVHWithLevel, 8> Stack(1, Root);
+  SmallSet<Value *, 8> VisitedInstrs;
+  bool Res = false;
+  while (!Stack.empty()) {
+    Value *V = Stack.back();
+    if (!V) {
+      Stack.pop_back();
----------------
ABataev wrote:
> anemet wrote:
> > This needs a description of the algorithm.
> The algorithm is the same as before, just exit criteria become a bit weaker.
There is a non-trivial loop that wasn't there before!?  Please explain what it does and what state the various new data structures represent. 


Repository:
  rL LLVM

https://reviews.llvm.org/D25517