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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper 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
----------------
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?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4155
+      // this case.
+      ParentStackElem.second = ParentStackElem.first->getNumOperands();
+    } else {
----------------
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.)


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4166
 
+  A
   /// \brief Try to find a reduction tree.
----------------
This looks out of place. 


================
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];
----------------
You can use ExtraArgs.lookup(TreeN) to replace the "if (map.count(k) v = map[k]" pattern. This saves a lookup.


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


https://reviews.llvm.org/D28961