[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:612
+  /// More details: http://www.llvm.org/devmtg/2015-10/slides/Porpodas-ThrottlingAutomaticVectorization.pdf
+  Optional<int> findSubTree(int Cost);
+
----------------
DO you really need to return `Optional` here? Maybe, just return `-SLPCostThreshold` if not successful?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1502-1503
     /// Do we need to gather this sequence ?
-    bool NeedToGather = false;
+    enum EntryState { Vectorize, NeedToGather, ProposedToGather };
+    EntryState State;
 
----------------
Could you split the patch and commit this part of the change (I mean, using of the enum instead of bool) as a separate NFC patch?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3251-3252
+        auto *UserInst = dyn_cast<Instruction>(U);
+        if (!UserInst)
+          continue;
+        if (!Removed.count(U))
----------------
Can this occur at all?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3905
 
-int BoUpSLP::getTreeCost() {
-  int Cost = 0;
+int BoUpSLP::getExtractOperationCost(const ExternalUser &EU) const {
+  unsigned BundleWidth = VectorizableTree.front()->Scalars.size();
----------------
The code in this function is very similar to the code in the `getTreeCost()`. Can it be reused somehow to avoid duplication?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3918
+  auto *VecTy = VectorType::get(EU.Scalar->getType(), BundleWidth);
+  Value *ScalarRoot = VectorizableTree[0]->Scalars[0];
+
----------------
`VectorizableTree.front()` instead of `VectorizableTree[0]` just like in the first statement.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4018
+      if (Next->State == TreeEntry::ProposedToGather ||
+          is_contained(Path, Next) || Next == C)
+        continue;
----------------
`is_contained()` is `O(n)`. Maybe use a set instead of it in the loop?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4147-4154
+  SmallString<256> Str;
+  raw_svector_ostream OS(Str);
+  OS << "SLP: Spill Cost = " << SpillCost << ".\n"
+     << "SLP: Extract Cost = " << ExtractCost << ".\n"
+     << "SLP: Total Cost = " << Cost << ".\n";
   LLVM_DEBUG(dbgs() << Str);
   if (ViewSLPTree)
----------------
All this code must be active only when the debug mode on?


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

https://reviews.llvm.org/D57779