[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:5523
+
+    // Partially vectorize trees after all full vectorization is done,
+    // otherwise, we could prevent more profitable full vectorization with
----------------
dtemirbulatov wrote:
> vporpo wrote:
> > I don't think throttling should be visible at this level. It should be called after the call to getTreeCost().
> I have seen so many regressions where partial vectorization prevents more profitable full vectorization and I think it is unacceptable to allow any and I think it is a good solution to those issues and It is not time-consuming we already done dependancy analysis at this point for all those seeds.  I will double check.
What about this?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2773
+    // Stop if we are over our budget of maximum cost calculations.
+    if (CostsRecalculations >= MaxCostsRecalculations)
+      break;
----------------
vporpo wrote:
> Keeping track of the budget here is a bit strange, because the budget tracking is internal state of throttling. Could you move this check somewhere better?
What about this?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2776
+
+    buildTree(S);
+
----------------
dtemirbulatov wrote:
> vporpo wrote:
> > buildTree() is an expensive operation because of scheduling. We should not build a new tree if we can reuse the existing one.
> > 
> I think it is the dependency analysis is expensive, when certain seeds were just ignored, I could get the numbers. But here, we have already correct seed from the dependency analysis point of view.  hmm, We could turn off the dependancy check for our collected seeds.
What about this?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3707-3712
+      if (CostsRecalculations >= MaxCostsRecalculations) {
+        SubPath.clear();
+        break;
+      }
+      if (PartialCost.hasValue())
+        return PartialCost;
----------------
Maybe, it is better to make the second `if` the first one and vice versa? Just in case, if we found a path with good cost even if maximum number of recalculations is reached?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3722-3726
+      if (NextFromBranch && NextFromBranch != Node) {
+        Node = findLeaf(NextFromBranch, Path);
+      } else {
+        Node->IsBranch = false;
+      }
----------------
No need for braces here


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3746
+  if (CostsRecalculations < MaxCostsRecalculations) {
+    for (unsigned I = 1, E = VectorizableTree.size(); I < E; ++I)
+      assert(VectorizableTree[I]->ProposedToGather && "Incorrect node state");
----------------
Could you use range-based loop here?


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

https://reviews.llvm.org/D57779