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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8513
   }
   auto *Exit = new VPBasicBlock(Twine(RegionName) + ".continue", PHIRecipe);
   auto *Pred = new VPBasicBlock(Twine(RegionName) + ".if", PredRecipe);
----------------
nit, unrelated to this patch: should this Exit also be Exiting?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8669
   if (IsVPlanNative)
-    EB->setCondBit(nullptr);
+    EB->back().eraseFromParent();
   EB->appendRecipe(CanonicalIVIncrement);
----------------
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()?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:389
+    assert(
+        getSingleSuccessor() &&
+        "block with multiple successors doesn't have a recipe as terminator");
----------------
VPBB's w/o successors - must not be empty?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:401
+
+  if (getNumSuccessors() < 2 && getParent()->getExitingBasicBlock() != this) {
+    assert(
----------------
Should some 'isLatch()' check if a VPBB is exiting its enclosing loop region?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:750
   }
+  case VPInstruction::BranchOnCond: {
+    Value *Cond = State.get(getOperand(0), VPIteration(Part, 0));
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:759
+    BranchInst *CondBr;
+    if (getParent() == ParentRegion->getExitingBasicBlock()) {
+      CondBr = Builder.CreateCondBr(Cond, Builder.GetInsertBlock(),
----------------
If isLatch()?
Have separate VPInstructions for forward and backward/loop-latch branches?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:764
+      CondBr = Builder.CreateCondBr(Cond, Builder.GetInsertBlock(), nullptr);
+    }
+    CondBr->setSuccessor(0, nullptr);
----------------
nit: can remove {}'s


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:734
+    }
+
+    if (getParent() == TopRegion->getExitingBasicBlock()) {
----------------
fhahn wrote:
> 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!
>> 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?

Sure; perhaps convert VPBranchOnMaskRecipe to VPInstruction, thereby having an opcode interval for terminating branch VPInstructions, in a follow-up patch.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2034
+  /// If the block has multiple successors, return the branch recipe terminating
+  /// the block. If there is only a single successor, return nullptr;
+  const VPRecipeBase *getTerminator() const;
----------------
or if there are no successors


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:222
       if (Br->isConditional())
         getOrCreateVPOperand(Br->getCondition());
 
----------------
Should BranchOnCond be created here instead of below in buildPlainCFG()?
Comment deserves updating.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:34
+      if (!VPV->getUnderlyingValue())
+        continue;
       Instruction *Inst = cast<Instruction>(VPV->getUnderlyingValue());
----------------
Why is this change needed now?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:60
+               VPI->getOpcode() == VPInstruction::BranchOnCount;
+      return isa<VPBranchOnMaskRecipe>(R);
+    }();
----------------
fhahn wrote:
> 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.
Sure, also follows LLVM-IR, except that successors are held in VPBB and their order must match the direction dictated by CondBit of BranchOnCond.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126618