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

Michael Kuperstein via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 30 17:16:35 PST 2017

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.


More information about the llvm-commits mailing list