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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:617
+  /// the subtree without extract cost, spill cost and etc.
+  std::pair<int, int> getTreeCost();
 
----------------
Better to split this function, one to have the raw cost, and the original one, that uses the first for the raw cost, that returns the full cost.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1527
+    /// The index containing the use of this entry.
+    TinyPtrVector<TreeEntry *> UseTreeIndices;
+
----------------
Give a better name, this is not indices.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1639
 
+    // Find nodes with more than one use.
+    bool isBranch() const {
----------------
1. It does not find anything, it checks for something. A better description is required.
2. Use `///` style of comment here.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1642
+      return llvm::count_if(UseTreeIndices, [this](TreeEntry *Next) {
+               return (Next->Idx != Idx && Next->State == TreeEntry::Vectorize);
+             }) > 1;
----------------
Check for a cyclic dependency? What we have something like A->B->A?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4004
+
+BoUpSLP::TreeEntry *
+BoUpSLP::findLeaf(TreeEntry *C, SetVector<TreeEntry *> &Path) const {
----------------
Better to return bool here and insert last found entry into the path.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4024
+  } while (NonGatherUse != 0);
+  return C;
+}
----------------
You don't insert last found non-gathering node to the path. Is this correct?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4027
+
+int BoUpSLP::findSubTree(int Cost) {
+  SetVector<TreeEntry *> Path;
----------------
Did you think about recalculating the cost of the reduced tree instead of using a scheme with cost subtraction? This looks to be a lot more natural way. I mean, in the same loop, where we try to vectorize the original tree, try to reduce the tree (+, maybe, check that the cost for the removed nodes is going to be negative) and then just recalculate the cost of the reduced tree? The current scheme looks overcomplicated. This is just a suggestion.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:6091-6096
+  } else {
+    if (SLPThrottling && R.findSubTree(CostSum) < -SLPCostThreshold) {
+        R.vectorizeTree();
+        return true;
+    }
   }
----------------
No need for `else` here, just `if (SLPThrottling && ...`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:6348-6349
         Changed = true;
+      } else {
+        if (SLPThrottling && R.findSubTree(CostSum) < -SLPCostThreshold) {
+            R.vectorizeTree();
----------------
`else if`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:6352
+            Changed = true;
+        }
       }
----------------
Missed moving to the next bundle, no?


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

https://reviews.llvm.org/D57779