[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:2297-2298
+
+    /// Tree values proposed to be vectorized.
+    ValueSet ScalarsToVec;
+
----------------
Why do you need this new set? You can get the result just by using `ScalarToTreeEntry` data member and checking the vectorization status of the corresponding `TreeEntry`.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2302
+    /// decided to stay in a scalar form.
+    ValueSet VecToScalars;
+
----------------
Seem to me, here is the same story just like with `ScalarsToVec`


================
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;
+
----------------
Currently is not used


================
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);
+  }
----------------
Why need to use a SmallVector and then sort it? Better to use a set with custom compare functor.


================
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.
----------------
Does it include the cost of all subtree or just this particular `Entry`?


================
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;
----------------
Why need to exclude `UserCost` here?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6185-6187
+  } else if (SLPThrottling && R.findSubTree())
+    R.saveTree();
 
----------------
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`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6486-6487
         Changed = true;
-      }
+      } else if (SLPThrottling && R.findSubTree(UserCost))
+        R.saveTree();
     }
----------------
Enclose substatement into braces since the substatement in `then` branch is in braces.


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

https://reviews.llvm.org/D57779