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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2773
+    // Stop if we are over our budget of maximum cost calculations.
+    if (CostsRecalculations >= MaxCostsRecalculations)
+      break;
----------------
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?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2776
+
+    buildTree(S);
+
----------------
buildTree() is an expensive operation because of scheduling. We should not build a new tree if we can reuse the existing one.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3530
+
+  // Walk across all leaves in order to reduce their paths. If we encounter
+  // another branch on the way from our leaf then consider the path from given
----------------
Collecting all paths is expensive, and I think we should not be doing this here just for removing some nodes from the tree. Also, you will end up visiting the same nodes over and over again (the ones close to the root). As I had described in a previous comment, I think we could get a good-enough tree cutting with a single top-down traversal of the tree. Did you find any examples where that would not work?


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

https://reviews.llvm.org/D57779