[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:8513
   }
   auto *Exit = new VPBasicBlock(Twine(RegionName) + ".continue", PHIRecipe);
   auto *Pred = new VPBasicBlock(Twine(RegionName) + ".if", PredRecipe);
----------------
Ayal wrote:
> nit, unrelated to this patch: should this Exit also be Exiting?
Updated in 08482830eb8a.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:401
+
+  if (getNumSuccessors() < 2 && getParent()->getExitingBasicBlock() != this) {
+    assert(
----------------
Ayal wrote:
> Should some 'isLatch()' check if a VPBB is exiting its enclosing loop region?
Thanks, I added an `isExiting` helper, which might be a bit more descriptive  and more in-line with the naming for the other helpers.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:759
+    BranchInst *CondBr;
+    if (getParent() == ParentRegion->getExitingBasicBlock()) {
+      CondBr = Builder.CreateCondBr(Cond, Builder.GetInsertBlock(),
----------------
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.


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


================
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;
----------------
Ayal wrote:
> or if there are no successors
Adjusted, thanks!


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

I also updated the comment.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126618