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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added a comment.

Correct me if I am wrong, but I think your algorithm can only remove the rightmost branch of the SLP graph (and maybe several of them if they are close by), as StopAt is a single index in VectorizableTree and we remove all nodes > StopAt.
I think this could be improved if we could find all possible entries where we could stop at, not just one of them. Then, in the final traversal, we could remove all branches starting from each of these nodes all the way to the leaves.
Also, I think the Delta in reduceTreeCost() should not be needed as we should run this regardless of whether the original tree is profitable or not.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2429
+  unsigned NumVectorized = 0;
+  int Sum = 0;
+
----------------
CostSum or something more descriptive.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2431
+
+  // Look at tree elements in backward to the top,
+  // First it should be gathering nodes and then
----------------
Please provide a high-level description of the algorithm. 
To my understanding you are traversing the tree top to bottom and you are collecting the cost of all the non-vectorizable nodes and  pushing into a vector when a vectorizable node is encountered. Therefore the vector contains the costs that could be saved once we remove the branch ?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2436
+    TreeEntry *Entry = &VectorizableTree[I];
+    if (!Entry->NeedToGather) {
+      NumVectorized++;
----------------
Shouldn't we check Entry->Cost > 0 instead ? That would also take into consideration the spill costs.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2438
+      NumVectorized++;
+      Sum += Entry->Cost;
+      Tree.push_back(Sum);
----------------
Not sure what is going on here. This line is the same in the else part (line 2442).  Move it out of the if ?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2458
+  // Canceling unprofitable elements.
+  if (StopAt > 0 && StopAt < (Tree.size() - 1)) {
+    NumVectorized = 0;
----------------
Why not skip these values of StopAt in the previous loop then, since that is the place which finds the good values of StopAt.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2483
+        NumVectorized++;
+        if (NumVectorized > StopAt) {
+          Entry->NeedToGather = true;
----------------
There is duplication here with the previous loop.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3675
+    if (!RemovedOprations.empty()) {
+      bool Removed = false;
+      for (int I: RemovedOprations) {
----------------
Removing things from the scheduler is very useful, so it is probably a good idea to place it in a separate function.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4855
       Changed = true;
+    } else {
+      // Try to reduce the tree to make it patrially vectorizable.
----------------
We should try to apply throttling even if it is profitable to vectorize.
For example, the SLP graph could have two branches: one super profitable, say with cost -10, and another one with cost +8. The total is -2, which is still profitable, but the best solution would be to remove the bad branch.

In that case the call to reduceTreeCost() should be moved to buildTree().


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

https://reviews.llvm.org/D57779