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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Still wonder if the exceptional case of top region's header VPBB could be dealt with better, if not entirely outside getOrCreateVPBB() then perhaps entirely inside it?



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:176
   Loop *LoopOfBB = LI->getLoopFor(BB);
-  if (!LoopOfBB)
+  if (!LoopOfBB || isHeaderBB(BB, TheLoop))
     return VPBB;
----------------
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.)


================
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) {
----------------
This assert to be removed now?


================
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");
----------------
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;
}
```


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


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158333