[PATCH] D33320: [SLP] Improve comments and naming of functions/variables/members, NFC.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4824-4825
+  // vectorization is not possible or not effective, and currently analyzed
+  // instruction is a binary operation, try to vectorize the operands, using DFS
+  // traversal order. If the operands were not vectorized, repeat the same
+  // procedure considering each operand as a possible root of the horizontal
----------------
anemet wrote:
> ABataev wrote:
> > anemet wrote:
> > > ABataev wrote:
> > > > anemet wrote:
> > > > > ABataev wrote:
> > > > > > anemet wrote:
> > > > > > > ABataev wrote:
> > > > > > > > anemet wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > anemet wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > anemet wrote:
> > > > > > > > > > > > > Again, which order?
> > > > > > > > > > > > DFS
> > > > > > > > > > > pre/in/post?
> > > > > > > > > > pre
> > > > > > > > > Okay, then please update the comment.  Also please answer my question below why we chose to track the edges with an iterative pre-order traversal.  If it's unnecessary, please fix or add a FIXME.  Thank you.
> > > > > > > > It simplifies limiting the tree traversal level of the function. We can use simple pre-order traversal, but still need to keep the tree node level within the Stack, for example, to limit the recursion level.
> > > > > > > I am not sure I see a major difference, do you have an example?
> > > > > > I'm just saying that there is no major difference. We won't win anything by using of pre-order traversal comparing with DFS preorder traversal. We will need to track the current tree depth to be able to cut off the vectorization process if the tree depth is more than `RecursionMaxDepth` (see line 4879).
> > > > > > I'm just saying that there is no major difference. We won't win anything by using of pre-order traversal comparing with DFS preorder traversal.
> > > > > 
> > > > > Now I am confused.  pre-order traversal *is* DFS pre-order traversal.
> > > > > 
> > > > > 
> > > > I'm sorry, I meant you're suggesting to change traversal to BFS.
> > > No it's still preorder.  My point was that for preorder unlike postorder you don't need to keep track of level of processing that has been performed on a node (i.e. getOperandIndex()).  See the code one comment down.
> > Yes, I understand this. But how to track the tree depth in your code? We need to limit the depth by the `RecusrsionMaxDepth`.
> The depth could be stored with each stack element -- that would still be simpler than keeping track of the operand.
> 
> It would also properly describe the intent; the depth is only maintained to control the recursion limit.
I believe this is a matter of taste. Besides, the same approach is used in `matchAssociativeReduction` function and I believe it is better to use the same approach in all functions, instead of using the different algorithms for implementation of the same idea.


https://reviews.llvm.org/D33320