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

Vasileios Porpodas via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 12 19:58:17 PDT 2019


vporpo added inline comments.


================
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
----------------
dtemirbulatov wrote:
> vporpo wrote:
> > dtemirbulatov wrote:
> > > dtemirbulatov wrote:
> > > > dtemirbulatov wrote:
> > > > > vporpo wrote:
> > > > > > 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?
> > > > > >Also, you will end up visiting the same nodes over and over again (the ones close to the root).
> > > > > Hmm, It should not visit, multi-times the same node. I will check again.
> > > > > >Did you find any examples where that would not work?
> > > > > yes, Many, it is definitely better with tree traverse.
> > > > >Did you find any examples where that would not work?
> > > > just look at the tests files for the SLP, with the single index it was just "slp-throttle.ll", definitely it is more robust.  
> > > >Also, you will end up visiting the same nodes over and over again (the ones close to the root).
> > > sure, we are not visiting the same node over and over again.
> > Could you point to some of the examples you found? I think the fast top-down approach should work well on tree-like graphs.
> What do you mean here with "top-down approach"? We are already here moving from leaves to the root of the graph and erasing nodes from leaves to the root in order to find satisfiable tree cost. 
What I am referring to is the simple approach that I had described where you don't follow paths, but instead you visit nodes one by one top-down. My point is that if we can avoid building paths, we should do so.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:557
+  int cutPath(int &Cost, SmallVectorImpl<unsigned> &Path,
+              SmallVectorImpl<unsigned> &VecNodes);
+
----------------
A general comment: Now that we have reliable pointers to TreeEntry, I think it is better to use TreeEntry * instead of an index to VectorizableTree.


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

https://reviews.llvm.org/D57779





More information about the llvm-commits mailing list