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

Adam Nemet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 2 09:49:00 PDT 2017


anemet accepted this revision.
anemet added a comment.
This revision is now accepted and ready to land.

Thanks very much for rewriting the loop.  This is way more intuitive now.

A few more nits/questions below but feel free to commit either way.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4783
+  // Interrupt the process if the Root instruction itself were vectorized or all
+  // sub-trees not higher than RecursionMaxDepth were analyzed/vectorized.
+  SmallVector<std::pair<WeakVH, unsigned>, 8> Stack(1, {Root, 1});
----------------
Capitalize sentence.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4784
+  // sub-trees not higher than RecursionMaxDepth were analyzed/vectorized.
+  SmallVector<std::pair<WeakVH, unsigned>, 8> Stack(1, {Root, 1});
   SmallSet<Value *, 8> VisitedInstrs;
----------------
I would start the level from 0, that's more canonical and then later would check like this:

if (++Level < MaxDepth)


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4801-4802
+          Res = true;
+          // Set P to nullptr to avoid re-analysis of phi node in
+          // matchAssociativeReduction function on the next iterations.
+          P = nullptr;
----------------
Rather than saying "on the next iteration", isn't it the desire that we don't analyze the phi node unless this is the root node?  If yes it's better to say so, i.e. "unless this is the root node".


https://reviews.llvm.org/D33320





More information about the llvm-commits mailing list