[PATCH] D123457: [VPlan] Initial modeling of middle block in VPlan.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Nice preparation for expanding VPlan's scope to include exit/middle-block after its expansion to include preheader.
Looks good to me, adding minor nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3673
+  VPBasicBlock *LatchVPBB = Plan.getVectorLoopRegion()->getExitBasicBlock();
+  Loop *VectorLoop = LI->getLoopFor(State.CFG.VPBB2IRBB[LatchVPBB]);
   // If we inserted an edge from the middle block to the unique exit block,
----------------
Calls for some VPLoopRegion2IRLoop API?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3871
+  VPBasicBlock *LatchVPBB =
+      PhiR->getParent()->getPlan()->getVectorLoopRegion()->getExitBasicBlock();
+  BasicBlock *VectorLoopLatch = State.CFG.VPBB2IRBB[LatchVPBB];
----------------
Can getEnclosingLoopRegion() of PhiR's parent? (it can go up to Plan to retrieve/assert it)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8772
   VPBlockUtils::insertBlockAfter(TopRegion, Preheader);
+  VPBasicBlock *MiddleBlock = new VPBasicBlock("middle.block");
+  VPBlockUtils::insertBlockAfter(MiddleBlock, TopRegion);
----------------
nit: "MiddleBlock" >> "MiddleVPBB" (or "ExitVPBB") for consistency?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:327
   };
 
+  if (getPlan()->getVectorLoopRegion()->getSingleSuccessor() == this) {
----------------
Deserves a comment, potentially starting with "0."

nit: can retain indentation as it should be an "else if"; perhaps better to switch and deal with exit block afterwards (should fail B) for a simpler "else if"("reusing" existing IR Exit Block can then be documented as "D")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123457