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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3286-3295
+  for (std::unique_ptr<TreeEntry> &TEPtr : Tree->VectorizableTree) {
+    TreeEntry *Entry = TEPtr.get();
+    if (Entry->State == TreeEntry::Vectorize)
+      VecNodes.push_back(Entry);
+  }
+  // Canceling unprofitable elements.
+  for (std::unique_ptr<TreeEntry> &TEPtr : Tree->VectorizableTree) {
----------------
These two loops can be merged, no? And use `switch` instead of `if`, if possible, after merging


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3289
+    if (Entry->State == TreeEntry::Vectorize)
+      VecNodes.push_back(Entry);
+  }
----------------
You don't need to push the elements to a new vector here, instead, you can directly perform required actions.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3865-3866
   // Gathering cost would be too much for tiny trees.
-  if (VectorizableTree[0]->State == TreeEntry::NeedToGather ||
-      VectorizableTree[1]->State == TreeEntry::NeedToGather)
+  if (Tree->VectorizableTree[0]->State == TreeEntry::NeedToGather ||
+      Tree->VectorizableTree[1]->State == TreeEntry::NeedToGather)
     return false;
----------------
Maybe, better to use `!= TreeEntry::Vectorize` to avoid trees with proposed gathering?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4084-4088
+      for (User *Op : Inst->users())
+        if (Tree->ScalarToTreeEntry.find(Op) != Tree->ScalarToTreeEntry.end()) {
+          NeedGather = true;
+          break;
+        }
----------------
`llvm::any_of(Inst->users(), [Tree](User *Op){ return Tree->ScalarToTreeEntry.count(Op) > 0; }`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4121
+  // Avoid reducing the tree if there is no potential room to reduce.
+  if ((Tree->TreeCost - UserCost - Sum) > -SLPCostThreshold)
+    return false;
----------------
`>=`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6419
       int Cost = R.getTreeCost();
+      unsigned UserCost = 0;
       CandidateFound = true;
----------------
Do you really need this new var here? I don't see where it is used except as an argument of `R.findSubTree(UserCost)` call


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4108-4111
+    int i = 0;
+    for (auto It = Vec.begin(), E = Vec.end(); It != E; ++It, i++)
+      if (i>MaxCostsRecalculations)
+        Vec.erase(It);
----------------
dtemirbulatov wrote:
> ABataev wrote:
> > Just `Vec.erase(Vec.rbegin(), Vec.rbegin() + (Vec.size() - MaxCostsRecalculations)`?
> No, We could not use "Vec.rbegin() + " with std::set.
Then just `Vec.erase(Vec.begin() + MaxCostsRecalculations, Vec.end());`.


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

https://reviews.llvm.org/D57779