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

[Dinar Temirbulatov via Phabricator via llvm-commits]

dtemirbulatov marked 9 inline comments as done.
dtemirbulatov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2307-2308
+
+    /// Total cost of inserts in the tree for a particular value.
+    SmallDenseMap<Value *, int> VecInserts;
+
----------------
ABataev wrote:
> Currently is not used
Thanks, Sorry for this, I missed somehow.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4108-4117
+  SmallVector<TreeEntry *, 64> Vec;
+  for (const std::unique_ptr<TreeEntry> &TEPtr : Tree->VectorizableTree) {
+    TreeEntry *Entry = TEPtr.get();
+    if (Entry->State != TreeEntry::Vectorize || Entry->Cost <= 0 || !Entry->Idx)
+      continue;
+    Vec.push_back(Entry);
+  }
----------------
ABataev wrote:
> Why need to use a SmallVector and then sort it? Better to use a set with custom compare functor.
Thanks, Good.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4124
+  for (TreeEntry *Entry : Vec)
+    Sum += Entry->Cost;
+  // Avoid reducing the tree if there is no potential room to reduce.
----------------
ABataev wrote:
> Does it include the cost of all subtree or just this particular `Entry`?
The Entry->Cost is a particular Entry Cost with some sub-tree elements for example if we have a gathering element in this particular Entry. Note that we only consider here TreeEntry::Vectorize entries this summary. 


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4126
+  // Avoid reducing the tree if there is no potential room to reduce.
+  if ((Tree->TreeCost - UserCost - Sum) > -SLPCostThreshold)
+    return false;
----------------
ABataev wrote:
> Why need to exclude `UserCost` here?
We might want to add(substract) extra value for example at line 6071 before this change.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6185-6187
+  } else if (SLPThrottling && R.findSubTree())
+    R.saveTree();
 
----------------
ABataev wrote:
> Actually, `else` is not required here at all. Just make it a standalone `if` statement since there is an early exit in the previous `if`
hmm, No I think it is required here, we don't  want  to reduce already decided full-tree vectorization.


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

https://reviews.llvm.org/D57779