[PATCH] D158333: [VPlan] Move initial skeleton construction to createInitialVPlan. (NFC)

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 6 inline comments as done.
fhahn added a comment.





================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:176
   Loop *LoopOfBB = LI->getLoopFor(BB);
-  if (!LoopOfBB)
+  if (!LoopOfBB || isHeaderBB(BB, TheLoop))
     return VPBB;
----------------
Ayal wrote:
> Better have getOrCreateVPBB() fully handle TheLoop's header (see below), and retain this early-exit only if !LoopOfBB? (Or explain its partial treatment here - region created (and registered in Loop2Region) before the call but hooking up VPBB as its entry/parent and fixing its name is left for the caller to fix after the call.)
Updated, thanks! The caller now doesn't add the region to Loop2Region, all headers are handled here now.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:180-181
+  bool RegionExists = Loop2Region.contains(LoopOfBB);
+  assert(RegionExists ^ isHeaderBB(BB, LoopOfBB) &&
          "region must exist or BB must be a loop header");
+  if (RegionExists) {
----------------
Ayal wrote:
> This assert to be removed now?
removed, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:183
+  if (RegionExists) {
+    assert(!isHeaderBB(BB, LoopOfBB) &&
+           "a region to use only exists for non-header blocks");
----------------
Ayal wrote:
> An alternative to early exiting, leaving setEntry to the caller, is to provide full treatment for top loop header BB; would something as follows work better, also taking care of updating Loop2Region?
> 
> 
> ```
>   auto *RegionOfVPBB = Loop2Region.lookup(LoopOfBB);
>   if (!isHeaderBB(BB, LoopOfBB)) {
>     assert(RegionOfVPBB && "Region should have been created by visiting header earlier");
>     VPBB->setParent(RegionOfVPBB);
>     return VPBB;
>   }
> 
>   // Handle a header - take care of its Region.
>   assert(!RegionOfVPBB && "...");
>   if (LoopOfBB == TheLoop) {
>     RegionOfVPBB = Plan.getVectorLoopRegion();
>   } else {
>     RegionOfVPBB =
>       new VPRegionBlock(BB->getName().str(), false /*isReplicator*/);
>     RegionOfVPBB->setParent(Loop2Region[LoopOfBB->getParentLoop()]);
>   }
>   RegionOfVPBB->setEntry(VPBB);
>   Loop2Region[LoopOfBB] = RegionOfVPBB;
>   return VPBB;
> }
> ```
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:326
+  VPRegionBlock *TheRegion = Plan.getVectorLoopRegion();
+  Loop2Region[TheLoop] = TheRegion;
+  BasicBlock *ThePreheaderBB = TheLoop->getLoopPreheader();
----------------
Ayal wrote:
> fold this into its natural place in getOrCreateVPBB()?
Done, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:357
   VPBlockBase *HeaderVPBB = getOrCreateVPBB(TheLoop->getHeader());
   HeaderVPBB->setName("vector.body");
+  TheRegion->setEntry(HeaderVPBB);
----------------
Ayal wrote:
> nit: cleaner to provide the name as an (optional) parameter to getOrCreateVPBB()?
Done, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:358
   HeaderVPBB->setName("vector.body");
-  ThePreheaderVPBB->setOneSuccessor(HeaderVPBB->getParent());
+  TheRegion->setEntry(HeaderVPBB);
 
----------------
Ayal wrote:
> fold this into its natural place in getOrCreateVPBB()?
Done, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158333