[PATCH] D28961: [SLP] Fix for PR31690: Allow using of extra values in horizontal reductions.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev marked 5 inline comments as done.
ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4152
+      // We ran into something like:
+      // ParentStackElem.first += ... + <ExtraArg_i> + ExtraArg + ...
+      // The whole ParentStackElem.first should be considered an extra value in
----------------
mkuper wrote:
> Isn't "ParentStackElim.first" in this case just "<ExtraArg_i> + ExtraArg" itself? What you're trying to say is that ParentStackElem.first has two "extra" arguments, so it is itself an extra argument, right?
Yes, I just provided some general description in C/C++ like format rather than in LLVM IR format.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4155
+      // this case.
+      ParentStackElem.second = ParentStackElem.first->getNumOperands();
+    } else {
----------------
mkuper wrote:
> What this basically does is avoid recursing above this node, right? Please document this, since it's non-obvious looking from this code ("second" here isn't meaningful, unless you know what the pair you were called with means - which is not documented above.)
Ok, will do


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4166
 
+  A
   /// \brief Try to find a reduction tree.
----------------
mkuper wrote:
> This looks out of place. 
Ooops, will remove it.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4219
+          // Check if TreeN is an extra argument of its parent operation.
+          if (ExtraArgs.count(TreeN)) {
+            Value *ExtraArg = ExtraArgs[TreeN];
----------------
mkuper wrote:
> You can use ExtraArgs.lookup(TreeN) to replace the "if (map.count(k) v = map[k]" pattern. This saves a lookup.
Ok, will do, thanks.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4229
+              // reduction operations.
+              markExtraArg(Stack[Stack.size() - 2], TreeN);
+              ExtraArgs.erase(TreeN);
----------------
mkuper wrote:
> Does "Stack.size() - 2" always point to the parent? Please document this.
Yes, will add some additional comment


https://reviews.llvm.org/D28961