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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

Seems to me, some old questions are still unanswered.



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3270-3272
+  // Estimate the subtree not just from a cost perspective, but functional.
+  if (VecNodes.size() < 3 || RealOpCount <= FlowOpCount)
+    return false;
----------------
Why do you need to compare flow and operation instructions count? Also, why use hardcoded `3` as a limit of vectorizable nodes?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3281-3284
+      for (Value *V : Entry->Scalars) {
+        LLVM_DEBUG(dbgs() << "SLP: Remove scalar " << *V
+                          << " out of proposed to vectorize.\n");
+      }
----------------
The scalars are not actually removed here.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4189
+  if (NoCallInst)
+    assert(getSpillCost() == 0 && "Incorrect spill cost");
+#endif
----------------
`SpillCost == 0`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6141-6145
+  } else {
+    if (SLPThrottling && R.findSubTree()) {
+      R.vectorizeTree();
+      return true;
+    }
----------------
`else if`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6398-6399
         Changed = true;
+      } else {
+        if (SLPThrottling && R.findSubTree(UserCost)) {
+          R.vectorizeTree();
----------------
`else if`


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

https://reviews.llvm.org/D57779