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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
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,
----------------
Ayal wrote:
> Calls for some VPLoopRegion2IRLoop API?
Sounds good, there's at least one other potential user. I'll do it as follow-up.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:327
   };
 
+  if (getPlan()->getVectorLoopRegion()->getSingleSuccessor() == this) {
----------------
Ayal wrote:
> 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")
Updated to reduce the indent and updated the comment. I added a separate comment for re-using ExitBB, as this is different to re-using the last BB. I also retained the oder of checks as this keeps the existing conditions simpler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123457