[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: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:
> 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.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1514
 
+  /// The index containing the use of this entry by other entries.
+  SmallDenseMap<unsigned, SmallVector<unsigned, 2>> UseTreeIndices;
----------------
Since these are operand edges for each TreeEntry, I think they should be  members of the TreeEntry struct, similarly to the UserTreeIndices.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3443
+void BoUpSLP::treeTraversal(SmallVectorImpl<unsigned> &Leaves,
+                            SmallVectorImpl<unsigned> &Branches) const {
+  SmallVector<unsigned, 16> Worklist;
----------------
You don't seem to be using the order of the entries in `branches` apart from checking is_contained() a couple of times. Isn't it better to add a member variable in TreeEntry for this instead?

Also I don't understand what is considered a `branch`. Is it any non-leaf node in the graph? Could you add some comment about it.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3448
+  Worklist.push_back(0);
+  // Does a BFS on non-gathering nodes of SLP graph and since the graph might
+  // have cycles we have to mark each visited node to process such tree.
----------------
Why is this a BFS ? Can't you visit the entries in the order they are pushed in VectorizableTree and collect the `Leaves` ?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3525
+      Idx = VectorizableTree[Idx]->NonGatherPred;
+      if (is_contained(Branches, Idx))
+        LeavesFromBranch[Idx].push_back(L);
----------------
This is linear time search in `Branches`. I think you should find a better way of keeping this data. For example you could add some member variables in TreeEntry.isBranch or something similar to speed things up.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3600
 
-    int C = getEntryCost(&TE);
-    LLVM_DEBUG(dbgs() << "SLP: Adding cost " << C
+    for (const EdgeInfo &UserTreeIdx : TE.UserTreeIndices)
+      UseTreeIndices[UserTreeIdx.UserTE->Idx].push_back(I);
----------------
Connecting the TreeEntries with edges should be part of building the graph, so I think a better place for this is somewhere in buildTree_rec().


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

https://reviews.llvm.org/D57779