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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4097
+bool BoUpSLP::findSubTree(int UserCost) {
+  auto cmp = [](const TreeEntry *LHS, const TreeEntry *RHS) {
+    return LHS->Cost > RHS->Cost;
----------------
`Cmp`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4108-4111
+    int i = 0;
+    for (auto It = Vec.begin(), E = Vec.end(); It != E; ++It, i++)
+      if (i>MaxCostsRecalculations)
+        Vec.erase(It);
----------------
Just `Vec.erase(Vec.rbegin(), Vec.rbegin() + (Vec.size() - MaxCostsRecalculations)`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4118
+  // Avoid reducing the tree if there is no potential room to reduce.
+  if ((Tree->TreeCost - UserCost - Sum) > -SLPCostThreshold)
+    return false;
----------------
`>=`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7259
+          Throttling = true;
+          Cost = V.getTreeCost() + ReductionCost;
+        }
----------------
Looks like you missed compare ща `Cost` with `-SLPCostThreshold` here. You vectorized the tree after throttling unconditionally. Plus, the `Cost` is calculated here, but not used later except for the debug prints.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6185-6187
+  } else if (SLPThrottling && R.findSubTree())
+    R.saveTree();
 
----------------
dtemirbulatov wrote:
> ABataev wrote:
> > Actually, `else` is not required here at all. Just make it a standalone `if` statement since there is an early exit in the previous `if`
> hmm, No I think it is required here, we don't  want  to reduce already decided full-tree vectorization.
Ho, you don't need it. Read https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


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

https://reviews.llvm.org/D57779