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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

Also, would be good to see some general description of this algorithm, without references to youtube.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3751
+    // vectorization decision.
+    if (RemovedOperations.size())
+      removeFromScheduling(BS);
----------------
vporpo wrote:
> if (!RemovedOperations.empty())
Not done. Please, review all previous comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2198
             assert(!UseEntry->NeedToGather && "Bad state");
+            InternalTreeUses[U].emplace_back(ExternalUser(Scalar, U, FoundLane));
             continue;
----------------
Just `emplace_back(Scalar, U, FOundLane);`.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2862-2871
+  for (TreeEntry *E : VecNodes) {
+    Instruction *Inst = E->getMainOp();
+    if (Inst && (isa<BinaryOperator>(Inst) || isa<FPMathOperator>(Inst) ||
+                 isa<CmpInst>(Inst))) {
+      FoundRealOp = true;
+      break;
+    }
----------------
Use `llvm::find_if` instead of this block of code.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2920
+  bool Changed = false;
+  for (SmallVector<Value *, 8> &S : Seeds) {
+    // Check those seed instructions are still alive.
----------------
Just `ArrayRef<Value *>`?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2940
+    if (Cost < -SLPCostThreshold) {
+      vectorizeTree();
+      Changed = true;
----------------
Some kind of logging here and in case of failed vectorization/too high cost?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2949
+void BoUpSLP::recordSeeds(ArrayRef<Value *> Ops) {
+  Seeds.push_back(SmallVector<Value *, 8>(Ops.begin(), Ops.end()));
+}
----------------
`emplace_back(Ops.begin(), Ops.end())`?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3548
+    bool Signed = It->second.second;
+    IntegerType *MinTy = IntegerType::get(F->getContext(), Width);
+    unsigned ExtOp = Signed ? Instruction::SExt : Instruction::ZExt;
----------------
`auto *`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3631-3632
+    NonGatherUse = 0;
+    for (auto I = C->UseTreeIndices.rbegin(), E = C->UseTreeIndices.rend();
+         I != E; ++I) {
+      TreeEntry *Next = *I;
----------------
Use `for (TreeEntry *Next : llvm::reverse(C->UseTreeIndices))` if possible.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3656-3662
+    for (TreeEntry *Next : TE->UseTreeIndices) {
+      if (Next->Idx == N || Next->NeedToGather)
+        continue;
+      NonGatherUse++;
+      if (NonGatherUse > 1)
+        TE->IsBranch = true;
+    }
----------------
Use `TE->IsBranch = llvm::count_if(TE->UseTreeIndices, ...) > 1;`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3666
+
+int BoUpSLP::findSubTree(int Cost) {
+  SmallVector<TreeEntry *, 2> Path;
----------------
Better to return `Optional<int>` and return `llvm::None` instead of `INT_MAX`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3678-3680
+    Node = Path.back();
+    if (Node->Idx && Node->ProposedToGather)
+      continue;
----------------
This might lead to the infinite loop since `Path` is not modified


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3694-3702
+      for (auto I = Node->UseTreeIndices.rbegin(),
+                E = Node->UseTreeIndices.rend();
+           I != E; ++I) {
+        TreeEntry *Next = *I;
+        if (Next == Node || Next->ProposedToGather || is_contained(Path, Next))
+          continue;
+        NextFromBranch = Next;
----------------
Use `llvm::find_if(llvm::reverse(Node->UseTreeIndices), ...)`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3764-3765
+    if (!TE.NeedToGather) {
+      for (Value *V : TE.Scalars)
+        ScalarsToVec.insert(V);
+    }
----------------
Use `llvm::copy`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3775-3781
   int ExtractCost = 0;
-  for (ExternalUser &EU : ExternalUses) {
-    // We only add extract cost once for the same scalar.
-    if (!ExtractCostCalculated.insert(EU.Scalar).second)
-      continue;
+  int SpillCost = 0;
+  int Cost = CostSum;
 
-    // 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;
-
-    // If we plan to rewrite the tree in a smaller type, we will need to sign
-    // extend the extracted value back to the original type. Here, we account
-    // for the extract and the added cost of the sign extend if needed.
-    auto *VecTy = VectorType::get(EU.Scalar->getType(), BundleWidth);
-    auto *ScalarRoot = VectorizableTree[0]->Scalars[0];
-    if (MinBWs.count(ScalarRoot)) {
-      auto *MinTy = IntegerType::get(F->getContext(), MinBWs[ScalarRoot].first);
-      auto Extend =
-          MinBWs[ScalarRoot].second ? Instruction::SExt : Instruction::ZExt;
-      VecTy = VectorType::get(MinTy, BundleWidth);
-      ExtractCost += TTI->getExtractWithExtendCost(Extend, EU.Scalar->getType(),
-                                                   VecTy, EU.Lane);
-    } else {
-      ExtractCost +=
-          TTI->getVectorInstrCost(Instruction::ExtractElement, VecTy, EU.Lane);
-    }
-  }
-
-  int SpillCost = getSpillCost();
-  Cost += SpillCost + ExtractCost;
+  ExtractCost = getExtractCost();
+  Cost += ExtractCost;
+  SpillCost = getSpillCost();
----------------
```
int ExtractCost = getExtractCost();
int SpillCost = getSpillCost();
int Cost = CostSum + ExtractCost;
```


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3784
 
   std::string Str;
+  if (CutTree) {
----------------
1. Use local `Str` in all branches
2. Switch to SmallString.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3791
+      return INT_MAX;
+    for (unsigned I = 0, E = VectorizableTree.size(); I < E; ++I) {
+      TreeEntry &TE = *VectorizableTree[I].get();
----------------
Use range-based loop


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

https://reviews.llvm.org/D57779