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

[Dinar Temirbulatov via Phabricator via llvm-commits]

dtemirbulatov marked 3 inline comments as done.
dtemirbulatov 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
----------------
ABataev wrote:
> 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?
Discussed at line #645


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


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2776
+
+    buildTree(S);
+
----------------
ABataev wrote:
> 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?
Discussed at line #645.


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

https://reviews.llvm.org/D57779