[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:2276
+  struct TreeState {
+    using TreeStateTy = SmallVector<std::unique_ptr<TreeState>, 8>;
+
----------------
Why need to store a pointer to `TreeState` but the `TreeState` itself?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3310-3312
+      for (Value *V : Entry->Scalars)
+        LLVM_DEBUG(dbgs() << "SLP: Remove scalar " << *V
+                          << " out of proposed to vectorize.\n");
----------------
This does nothing except for debugging print, guard with `#ifndef NDEBUG .. #endif` 


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4113
+  }
+  llvm::sort(Vec, [&](const TreeEntry *LHS, const TreeEntry *RHS) {
+    return LHS->Cost > RHS->Cost;
----------------
No need for `[&]` here, just `[]`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4136
+          llvm::remove_if(Tree->ExternalUses,
+                          [&V](ExternalUser &EU) { return EU.Scalar == V; }),
+          Tree->ExternalUses.end());
----------------
`[V]`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4224-4227
+#ifndef NDEBUG
+  if (Tree->NoCallInst)
+    assert(getSpillCost() == 0 && "Incorrect spill cost");
+#endif
----------------
Just `assert((!Tree->NoCallInst || getSpillCost() == 0) && "Incorrect spill cost");`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4747
       if (getTreeEntry(PO))
-        ExternalUses.push_back(ExternalUser(PO, cast<User>(VecPtr), 0));
+        Tree->ExternalUses.push_back(ExternalUser(PO, cast<User>(VecPtr), 0));
 
----------------
Use `emplace_back(PO, VecPtr, 0)`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4792-4793
       if (getTreeEntry(ScalarPtr))
-        ExternalUses.push_back(ExternalUser(ScalarPtr, cast<User>(VecPtr), 0));
+        Tree->ExternalUses.push_back(
+            ExternalUser(ScalarPtr, cast<User>(VecPtr), 0));
 
----------------
Use `emplace_back(ScalarPtr, VecPtr, 0);`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4895
       if (ScalarArg && getTreeEntry(ScalarArg))
-        ExternalUses.push_back(ExternalUser(ScalarArg, cast<User>(V), 0));
+        Tree->ExternalUses.push_back(ExternalUser(ScalarArg, cast<User>(V), 0));
 
----------------
Use `emplace_back(ScalarArg, V, 0);`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5150-5154
+  BuiltTrees.erase(llvm::remove_if(BuiltTrees,
+                                   [&](std::unique_ptr<TreeState> &T) {
+                                     return T.get() != Tree;
+                                   }),
+                   BuiltTrees.end());
----------------
Why do you need to call `BuiltTrees.erase(` after `llvm::remove_if`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5151
+  BuiltTrees.erase(llvm::remove_if(BuiltTrees,
+                                   [&](std::unique_ptr<TreeState> &T) {
+                                     return T.get() != Tree;
----------------
`[Tree]`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5645
+    BS->doForAllOpcodes(I,
+                        [&](ScheduleData *SD) { SD->clearDependencies(); });
+  }
----------------
`[]`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6191-6192
     return true;
-  }
+  } else if (SLPThrottling && R.findSubTree())
+    R.saveTree();
 
----------------
Why not try to vectorize a partial tree right here?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6492-6493
         Changed = true;
-      }
+      } else if (SLPThrottling && R.findSubTree(UserCost))
+        R.saveTree();
     }
----------------
Why not try to vectorize a partial tree right here?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7281-7282
+          Cost = V.getTreeCost() + ReductionCost;
+        }
+        if (!Throttling) {
           V.getORE()->emit([&]() {
----------------
Just `else`?


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

https://reviews.llvm.org/D57779