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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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
----------------
Ayal wrote:
> nit: worth adding a note to revisit this comment, unless fully agreed upon?
Added, thanks! 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:171
+  if (Name.size() == 0)
+    Name = BB->getName();
+  LLVM_DEBUG(dbgs() << "Creating VPBasicBlock for " << Name << "\n");
----------------
Ayal wrote:
> 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.
Adjusted, thanks!


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


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


================
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
----------------
Ayal wrote:
> 
Adjusted, thanks!


================
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();
----------------
Ayal wrote:
> 
Adjusted, thanks!


================
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");
 
----------------
Ayal wrote:
> 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?
Removed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158333