[PATCH] D126618: [VPlan] Replace CondBit with BranchOnCond VPInstruction.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:734
+    }
+
+    if (getParent() == TopRegion->getExitingBasicBlock()) {
----------------
Ayal wrote:
> Similar to VPBranchOnMaskRecipe (have all terminator recipes share this somehow?):
>   // Replace the temporary unreachable terminator with a new conditional branch,
>   // hooking it up to backward destination now and to forward destination(s) later when they are created.
> 
> Handle loop branches of all loops, not only outermost?
> 
> Set exit successor to nullptr to be set later, rather than Builder.GetInsertBlock()?
> Similar to VPBranchOnMaskRecipe (have all terminator recipes share this somehow?):

Not sure how to best do this at the moment. Might be worth introducing a dedicated terminator subclass once things settle down a bit?

> Handle loop branches of all loops, not only outermost?

Done, removed the VPlan native code and also use parent region rather than topmost.

> Set exit successor to nullptr to be set later, rather than Builder.GetInsertBlock()?

Done, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:60
+               VPI->getOpcode() == VPInstruction::BranchOnCount;
+      return isa<VPBranchOnMaskRecipe>(R);
+    }();
----------------
Ayal wrote:
> May be useful to have an isaTerminator(Recipe).
> 
> Can have VPBB hold its 'singleton' terminator recipe (instead of CondBit) separately from all other recipes, analogous to the idea for Region to hold its singleton canonical IV recipe directly.
I added a `VPBasicBlock::getTerminator()` helper. Not sure if it is worth adding state to track the terminator, as it is trivial to access them for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126618