[PATCH] D121621: [VPlan] Track current vector loop in VPTransformState (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

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

This is fine; adding a clarifying comment.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:914
+  State->CurrentVectorLoop = L;
   State->CFG.LastBB = L->getExitBlock();
 
----------------
fhahn wrote:
> Ayal wrote:
> > Ayal wrote:
> > > Is CFG.LastBB now redundant, retrievable from CurrentVectorLoop->getExitBlock()?
> > > (Or could CurrentVectorLoop be retrieved from LastBB'd predecessor, if unique.)
> > (State->CurrentVectorLoop itself is also caching of State->LI->getLoopFor(State->CFG.VectorHeaderBB))
> > Is CFG.LastBB now redundant, retrievable from CurrentVectorLoop->getExitBlock()?
> 
> Unfortunately this only works if we maintain a correct branch to the exit block throughout VPlan execution. But we change the header branch to unreachable, so the won't work I think.
> 
> > (Or could CurrentVectorLoop be retrieved from LastBB'd predecessor, if unique.)
> > (State->CurrentVectorLoop itself is also caching of State->LI->getLoopFor(State->CFG.VectorHeaderBB))
> 
> The main motivation for this change is to be able to create new loop objects for each non-replicator region during VPRegionBlock::execute. At that point, no basic blocks for the loop exist and when creating the first block in the loop (in VPBasicBlock::execute), there's no previous block we could use to get the loop object. We could change VPRegionBlock::execute to create a dummy block on entry, but keeping track of the loop object explicitly seems cleaner to me.
> 
(This now relates to CFG.ExitBB instead of CFG.LastBB)

Ok, right; disconnecting the branch from VectorHeaderBB to middleBlock below will invalidate L->getExitBlock(), so ExitBB needs to be recorded above.

The current skeleton still creates VectorPreHeaderBB, VectorHeaderBB, and ExitBB/middleBlock, right? That's how L is retrieved above; the comment only clarifies that CurrentVectorLoop currently caches this retrieval (which should still be valid via the cumbersome
State->LI->getLoopFor(State->CFG.VectorPreHeader->getSingleSuccessor()))).
When that changes, and VectorHeaderBB is created during VPlan::execute() etc., the "Assumes a single LoopVectorBody basic-block was created" comment above should also be updated.

(In any case, no dummy blocks intended)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121621