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

[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!
Adding minor nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8669
   if (IsVPlanNative)
-    EB->setCondBit(nullptr);
+    EB->back().eraseFromParent();
   EB->appendRecipe(CanonicalIVIncrement);
----------------
fhahn wrote:
> Ayal wrote:
> > Seems somewhat awkward for addCanonicalIVRecipes() to take care of erasing a redundant terminator; could this cleanup take place when VPlanNative creates said terminator, before it calls addCanonicalIVRecipes()?
> I tried remove this when building the native plan, but this leaves the plan in an inconsistent state (exiting block without cond terminator) which triggers asserts with `getTerminator` calls. I move the code to remove it to the caller of this function.
This is fine, thanks.

Perhaps native should addCanonicalIVRecipes from within its VPInstructionsToVPRecipes.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:427
+bool VPBasicBlock::isExiting() const {
+  return getParent()->getExitingBasicBlock() == this;
+}
----------------
Does 'exiting' here refer to loop-exiting/latch or to region-exiting? I.e., are replicating region exits included or excluded.





================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:389
+    assert(
+        getSingleSuccessor() &&
+        "block with multiple successors doesn't have a recipe as terminator");
----------------
fhahn wrote:
> Ayal wrote:
> > VPBB's w/o successors - must not be empty?
> I think in theory it could be possible in native path, at least I don't think that's an enforced invariant?
Relax assert to check for no more than a single successor?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:750
   }
+  case VPInstruction::BranchOnCond: {
+    Value *Cond = State.get(getOperand(0), VPIteration(Part, 0));
----------------
fhahn wrote:
> Ayal wrote:
> > When is this called? Expecting Predication to replace forward BranchOnCond by Blend or BranchOnMask's for UF>1 and/or VF>1, and backward BranchOnCond to be replaced by BranchOnCount?
> > 
> > Assert here that Part==0?
> It is called for different parts, but everything worked because it always removed the previous terminator. Updated to break on part != 0.
ok, this implies condition must be uniform, may be good to assert so.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:759
+    BranchInst *CondBr;
+    if (getParent() == ParentRegion->getExitingBasicBlock()) {
+      CondBr = Builder.CreateCondBr(Cond, Builder.GetInsertBlock(),
----------------
fhahn wrote:
> Ayal wrote:
> > If isLatch()?
> > Have separate VPInstructions for forward and backward/loop-latch branches?
> Updated to use `isExiting`. I'm not sure if there's any benefit at the moment to introduce different recipes.
Ok. A thought to keep in mind.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:317
 
       // Link successors using condition bit.
+      VPBB->setTwoSuccessors(SuccVPBB0, SuccVPBB1);
----------------
w/o the using condition bit bit.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:34
+      if (!VPV->getUnderlyingValue())
+        continue;
       Instruction *Inst = cast<Instruction>(VPV->getUnderlyingValue());
----------------
fhahn wrote:
> Ayal wrote:
> > Why is this change needed now?
> BranchOnCount are VPInstructions without underlying values. I updated the code to just skip terminators.
Ahh, fine, very good.

nit: there's not much to continue after reaching the terminator; can iterate excluding terminator similar to excluding phi's.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:54
     // Check block's condition bit.
     if (VPB->getNumSuccessors() > 1 || Region->getExitingBasicBlock() == VPB)
+      assert(VPBB && VPBB->getTerminator() &&
----------------
`|| isExiting(VPB)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126618