[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/LoopVectorize.cpp:8669
   if (IsVPlanNative)
-    EB->setCondBit(nullptr);
+    EB->back().eraseFromParent();
   EB->appendRecipe(CanonicalIVIncrement);
----------------
Ayal wrote:
> 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.
Will do as follow-up!


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:389
+    assert(
+        getSingleSuccessor() &&
+        "block with multiple successors doesn't have a recipe as terminator");
----------------
Ayal wrote:
> 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?
Yes, adjusted!


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:34
+      if (!VPV->getUnderlyingValue())
+        continue;
       Instruction *Inst = cast<Instruction>(VPV->getUnderlyingValue());
----------------
Ayal wrote:
> 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.
Updated, thanks!


================
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() &&
----------------
Ayal wrote:
> `|| isExiting(VPB)`?
Updated thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126618