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

[Dinar Temirbulatov via Phabricator via llvm-commits]

dtemirbulatov marked 11 inline comments as done.
dtemirbulatov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2276
+  struct TreeState {
+    using TreeStateTy = SmallVector<std::unique_ptr<TreeState>, 8>;
+
----------------
ABataev wrote:
> Why need to store a pointer to `TreeState` but the `TreeState` itself?
TreeState is a large structure, it is convenient with dynamically allocate, but static version might be faster, do you think it is critical?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5150-5154
+  BuiltTrees.erase(llvm::remove_if(BuiltTrees,
+                                   [&](std::unique_ptr<TreeState> &T) {
+                                     return T.get() != Tree;
+                                   }),
+                   BuiltTrees.end());
----------------
ABataev wrote:
> Why do you need to call `BuiltTrees.erase(` after `llvm::remove_if`?
it is SmallVector<std::unique_ptr<TreeState>, 8> and we have to call erase(...)


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5151
+  BuiltTrees.erase(llvm::remove_if(BuiltTrees,
+                                   [&](std::unique_ptr<TreeState> &T) {
+                                     return T.get() != Tree;
----------------
ABataev wrote:
> `[Tree]`
Tree is a class property, not a local variable.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7281-7282
+          Cost = V.getTreeCost() + ReductionCost;
+        }
+        if (!Throttling) {
           V.getORE()->emit([&]() {
----------------
ABataev wrote:
> Just `else`?
We might try partial vectorization without success here and we to report about insufficient cost and break  


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

https://reviews.llvm.org/D57779