[PATCH] D126680: [VPlan] Replace BranchOnCount with BranchOnCond if TC <= UF * VF.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, thanks!

Added some nits and potential follow-ups.

No need to add a test for #55354, as it is already covered by existing tests, right?



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:751
+      break;
+
     Value *Cond = State.get(getOperand(0), VPIteration(Part, 0));
----------------
Independent of this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:890
+  auto *Term = dyn_cast<VPInstruction>(&ExitingVPBB->back());
+  // Try to simplify BranchOnCount to 'BranchOnCond true' if TC <= VF * UF.
+  if (Term && Term->getOpcode() == VPInstruction::BranchOnCount) {
----------------
CanonicalIVStartValue is assumed to be non negative, right?

This is as close to a VPlan2VPlan transformation as we can get at the moment in general. The TC <= VF case (using lower-bound of 1 for UF) could be optimized as an early VPlan2VPlan transformation, considering VPlan's VF range. But UF is currently set after finalizing a VPlan, except if set manually and/or optimizing for size.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:892
+  if (Term && Term->getOpcode() == VPInstruction::BranchOnCount) {
+    if (auto *C = dyn_cast<ConstantInt>(TripCountV)) {
+      uint64_t TCVal = C->getZExtValue();
----------------
nit: can fold condition e.g. into `&& isa<ConstantInt>`...


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:894
+      uint64_t TCVal = C->getZExtValue();
+      if (TCVal && TCVal <= State.VF.getKnownMinValue() * State.UF) {
+        auto *BOC =
----------------
getFixedValue()?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:897
+            new VPInstruction(VPInstruction::BranchOnCond,
+                              {getOrAddExternalDef(State.Builder.getTrue())});
+        Term->eraseFromParent();
----------------
(Could alternatively set the two operands of Term to the same value for trivial comparison, given that BranchOnCount actually means ReiterateWhileNotEqual. Currently BranchOnCond can also represent loop branches, and using it is clearer. May be good to have a single representation for loop branches, independent of this patch.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126680