[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:714
+
+    /// Full cost on expanding tree at this hight.
+    int ExpandAtCost = 0;
----------------
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.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:718
+    /// Parent operatrion of this operations.
+    Value *Parent = nullptr;
   };
----------------
I don't think we should add an extra Value *Parent field here.
There are many reasons for this:
1) The TreeEntry represents a group of scalars, so the parent should also be a TreeEntry node, not a Value*.
2) The "Parent" is actually the user of the TreeEntry, and we already have this information in UserTreeIndices. It is a vector of the indexes of the parent nodes (there may be more than one "parent" nodes, as the graph is a DAG, not a tree).



================
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,
----------------
Same here, do we really need the parent?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2691
+int BoUpSLP::getTreeCost(bool ReduceTree) {
+  SmallDenseMap<Value *, SmallVector<int, 8>> GatherMap;
+  unsigned NonGatherNum = 0;
----------------
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 ?



================
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];
----------------
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 ?


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



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

https://reviews.llvm.org/D57779