[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:122-130
+static cl::opt<bool>
+    SLPThrottling("slp-throttle", cl::init(true), cl::Hidden,
+                  cl::desc("Enable tree partial vectorize with throttling"));
+
+static cl::opt<unsigned>
+    MaxCostsRecalculations("slp-throttling-budget", cl::init(32), cl::Hidden,
+                           cl::desc("Limit the total number of nodes for cost "
----------------
Do we really need both of these options? `MaxCostsRecalculations` should be enough.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:628-632
+  ///   <bundle 1> scalar form
+  ///      |
+  ///   <bundle 2> scalar form  <bundle 3> scalar form
+  ///       \                    /
+  ///        <seed root bundle> scalar form
----------------
Does "scalar form" means "gathered nodes"? I don't think that currently we may end up with the situation like in the picture, we can't have gathered node that depends on another node (either gather or vectorized).


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:672-676
+  /// Save current tree for possible later vectorization.
+  void saveTree() {
+    BuiltTrees.push_back(std::make_unique<TreeState>());
+    Tree = BuiltTrees.back().get();
   }
----------------
Why do we need to save intermediate results? Cannot it be solved in a single iteration loop without saving the intermediate results in the class instance?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2723-2725
+            assert((UseEntry->State == TreeEntry::Vectorize ||
+                    UseEntry->State == TreeEntry::ScatterVectorize) &&
+                   "Bad state");
----------------
Looks like unrelated change


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

https://reviews.llvm.org/D57779