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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks for accommodating! A couple of final cleanups and nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8783
 
   // Don't use getDecisionAndClampRange here, because we don't know the UF
   // so this function is better to be conservative, rather than to split
----------------
nit: worth adding a note to revisit this comment, unless fully agreed upon?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:171
+  if (Name.size() == 0)
+    Name = BB->getName();
+  LLVM_DEBUG(dbgs() << "Creating VPBasicBlock for " << Name << "\n");
----------------
Alternatively, can check if isHeaderBB(BB, TheLoop) and set its name to "vector.body". This method already handles this case, and taking care of the name here would simplify and unify the caller(s) below. 

Would be good if these debug messages were covered by some test.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:189
+
+  assert(!RegionOfVPBB && "...");
+  // Handle a header - take care of its Region.
----------------
... should be filled in with some message, e.g., "First visit of a header basic block expects to register its region"


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:195
+    RegionOfVPBB =
+        new VPRegionBlock(BB->getName().str(), false /*isReplicator*/);
     RegionOfVPBB->setParent(Loop2Region[LoopOfBB->getParentLoop()]);
----------------
?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:324
+  // skeleton. These were created directly rather than via getOrCreateVPBB(),
+  // revisit them now to update BB2VPBB and Loop2region. Note that header/entry
+  // and latch/exiting VPBB's of top-level region have yet to be created
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:325
+  // revisit them now to update BB2VPBB and Loop2region. Note that header/entry
+  // and latch/exiting VPBB's of top-level region have yet to be created
+  VPRegionBlock *TheRegion = Plan.getVectorLoopRegion();
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:355-356
   }
-  // Create region (and header block) for the outer loop, so that we can link
-  // PH->Region.
-  VPBlockBase *HeaderVPBB = getOrCreateVPBB(TheLoop->getHeader());
-  HeaderVPBB->setName("vector.body");
-  ThePreheaderVPBB->setOneSuccessor(HeaderVPBB->getParent());
+  // Create empty VPBB for header block of the top region and set its name.
+  getOrCreateVPBB(TheLoop->getHeader(), "vector.body");
 
----------------
If getOrCreateVPBB() takes care of all blocks equally, creating VPBB for header block of top region can be done as part of the RPO scan below, along with all other VPBB's?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158333