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

[Dinar Temirbulatov via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:714
+
+    /// Full cost on expanding tree at this hight.
+    int ExpandAtCost = 0;
----------------
vporpo wrote:
> It is not straight-forward what "expanding" refers to. 
> Do you mean "scalarize" everything above this node? 
> If so, a variable name like ScalarizeAtCost would be better?
> (Also typo: hight->height)
> 
> Please rephrase this comment. It should be clear that it refers to the reduction of the tree with throttling.
Done.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:722
   /// Create a new VectorizableTree entry.
-  void newTreeEntry(ArrayRef<Value *> VL, bool Vectorized, int &UserTreeIdx,
+  void newTreeEntry(Value *Parent, ArrayRef<Value *> VL, bool Vectorized,
+                    int &UserTreeIdx,
----------------
vporpo wrote:
> Same here, do we really need the parent?
Fixed


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2691
+int BoUpSLP::getTreeCost(bool ReduceTree) {
+  SmallDenseMap<Value *, SmallVector<int, 8>> GatherMap;
+  unsigned NonGatherNum = 0;
----------------
vporpo wrote:
> I don't think a map is needed.
> You are mapping an instruction to a vector of the TreeEntry indexes of its operands that need to gather, but I think there are better ways to get this data.
> For example, you could replace the code of line 2749 with something along these lines:
> 
> ```
> for (Value *Op : V->operands())
>    if (TreeEntry *OpTE = getTreeEntry(Op))
>      GatherCost += OpTE->Cost;
> ```
> If it is really needed here, it needs a better name, maybe GatheringOperandsMap ?
> 
Still, I think we need the map. BTW, getTreeEntry() finds only vectorizable elements.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2733
 
-    // Uses by ephemeral values are free (because the ephemeral value will be
-    // removed prior to code generation, and so the extraction will be
-    // removed as well).
-    if (EphValues.count(EU.User))
-      continue;
+  for (unsigned I = 0, E = VectorizableTree.size(); I < E; I++) {
+    TreeEntry *Entry = &VectorizableTree[I];
----------------
vporpo wrote:
> Shouldn't we guard this whole loop with if (ReduceTree) ?
> 
> I find the code in this loop very confusing and hard to follow.
> Shouldn't you be doing a reverse-post-order traversal of the VectorizableTree accumulating the cost of the operands of each node and scalarizing all the nodes from that point on if the accumulated cost > threshold ?
It is the main tree cost calculation, I isolated ReduceTree code even further.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2743
+        if (GatherMap.find(V) != GatherMap.end())
+          for (int Gather : GatherMap[V])
+            GatherCost += VectorizableTree[Gather].Cost;
----------------
vporpo wrote:
> Avoid two lookups in GatherMap.find() and GatherMap[V], use an iterator.
> 
Fixed.


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

https://reviews.llvm.org/D57779