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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:645
+  /// Save seed instructions to try partially vectorize later.
+  void recordSeeds(ArrayRef<Value *> Ops);
+
----------------
Why are we collecting the seeds specifically for partial vectorization? Is this really needed? Why don't you just call tryPartialVectorization(Seeds) within vectorizeStorChain() etc. ?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1278
+    /// True if this node has more than one non-gathering child.
+    bool IsBranch = false;
+
----------------
Do we need a separate variable for this? Can't we replace it with a small isBranch() function that checks the operands?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1297
+    /// The index containing the use of this entry.
+    SmallVector<TreeEntry *, 1> UseTreeIndices;
+
----------------
Hmm is this needed? Isn't this available in EdgeInfo of UserTreeIndices?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3584
+
+int BoUpSLP::getExtractCost() const {
+  int ExtractCost = 0;
----------------
Do we need separate getExtractCost() , getSpillCost() , getInsertCost() functions? Can't we update the getTreeCost() function instead to be aware of tree cutting?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3672
+
+void BoUpSLP::treeTraversal() {
+  // Find nodes with more than one use and it might include cycles because
----------------
I think we could avoid this traversal completely by marking the nodes when the tree is being built in buildTree_rec(). For example, the TE->ProposedToGather could be set in newTreeEntry().


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

https://reviews.llvm.org/D57779