[PATCH] D57779: [SLP] Add support for throttling.

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added a comment.

Please try not to introduce a new Node structure or a new graph. You can use TreeEntry instead and you can add any necessary fields to it.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:542
+  /// \returns the cost of gathering canceled elements to be used
+  /// by vectorized  operations during throtelling.
+  int getInsertCost(const SmallPtrSetImpl<Value *> &VecToScalars);
----------------
Typo: throttling


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:545
+
+  /// Determine the cost of partail vectorization.
+  int getSubTreeCost(ArrayRef<unsigned> VecNodes);
----------------
Typo: partial


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1389
 
+  struct Node {
+    Node(unsigned I) : Idx(I) {}
----------------
Why not use the TreeEntry directly? It is already a graph node, with user edges and it already supports GraphTraits for traversals etc.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1398
+
+    /// Color of this node.
+    unsigned Color = 0;
----------------
What is the color used for? Please describe.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3444
+
+void BoUpSLP::BFS() {
+  std::vector<unsigned> Worklist;
----------------
Please use a more descriptive name here, as this function does more things than just a BFS. It is also connecting the nodes?
Also add a brief description comment to all functions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57779/new/

https://reviews.llvm.org/D57779