[PATCH] D125287: [SLP] Improve root steering by building actual trees instead of calling the look-ahead heuristic

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2028-2036
+      SmallVector<Value *, 2> Roots(
+          {Candidates[I].first, Candidates[I].second});
+      buildTree(Roots, /*UserIgnoreList=*/None,
+                RootLookAheadMaxSize.getValue());
+      if (isTreeTinyAndNotFullyVectorizable())
+        continue;
+      reorderTopToBottom();
----------------
ABataev wrote:
> vdmitrie wrote:
> > ABataev wrote:
> > > vdmitrie wrote:
> > > > ABataev wrote:
> > > > > vporpo wrote:
> > > > > > vdmitrie wrote:
> > > > > > > ABataev wrote:
> > > > > > > > I'm afraid of increasing compile time. All this stuff includes scheduling, which may take lots of time for large basic blocks.
> > > > > > > Yep, I agree. this will be more expensive for compile time.
> > > > > > > What about combining both worlds? I mean first try to use lookahead heuristics to get the single best. And if we can't narrow down to just one pair only then switch into probing via building trees.
> > > > > > > I believe it will not happen too frequently. We can also increase lookahead depth to make it even less frequent when we need to build vectorizable tree.
> > > > > > What if we set a flag to disable scheduling for these types of fast tree estimations? 
> > > > > There is a problem with this fix that it tries to avoid/mask the problem, not fix it. The fact that LookAhead.getScoreAtLevelRec does not work here means that we're doing something wrong there or missing something. Would be good to try to improve LookAhead.getScoreAtLevelRec
> > > > This problem does actually not have a perfect solution. This is a heuristics and it will always have something missed.
> > > > You can improve it to fix one particular case but there will be eventually another instance of the same problem.
> > > > 
> > > > 
> > > Yes, sure. But if the heuristic misses something, better to try tweak the heuristic rather than using actual cost/vectorization attempt and just ignore the heuristic, which exists exactly for this purpose.
> > No, " just ignore" is not what I said.  We definitely should use the heuristics. But when it happens that we came to its limits then we could use more fine grained tools.
> I rather doubt that building a graph can be called a "fine grained tool". This is different tool, not intended for the analysis. We can extract some functionality out of there (to a separate function/member function) and make the heuristic more smart, but not use the build graph directly.
> Same problem with the heuristic may exist in some other places, we need to handle them too.
Cost modeling is the tool I referred to as a "fine grained tool". We have to build a graph to run it. So it's sort of necessary evil. In this sense trying to turn off scheduler for the purpose of using CM as finer grained heuristics does not sound like a crazy idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125287