[PATCH] D123005: [VPlan] Use region for each loop in native path.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:89
+    auto *PredVPBB = getOrCreateVPBB(Pred);
+    VPBBPreds.push_back(PredVPBB);
+  }
----------------
Is this change needed?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:113
 // Create a new empty VPBasicBlock for an incoming BasicBlock or retrieve an
 // existing one if it was already created.
 VPBasicBlock *PlainCFGBuilder::getOrCreateVPBB(BasicBlock *BB) {
----------------
Also comment that if \p BB belongs to a loop (except for preheader - said loop needs to be in the scope of VPlan?), then the new empty VPBB is placed in a corresponding VPRegion - if exists otherwise in a new one.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:120
 
+  // Get or create a region for the loop containing BB.
+  Loop *CurrentLoop = LI->getLoopFor(BB);
----------------
Deserves a separate getOrCreate[EnclosingLoop]Region()?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:135
   BB2VPBB[BB] = VPBB;
-  VPBB->setParent(TopRegion);
+  VPBB->setParent(ParentR);
   return VPBB;
----------------
Would be good to add a test with nested loops showing how multiple regions are now being created, whereas until now only the outermost loop was region'd.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:267
          "Unexpected loop preheader");
   VPBasicBlock *PreheaderVPBB = getOrCreateVPBB(PreheaderBB);
   for (auto &I : *PreheaderBB) {
----------------
PreheaderVPBB >> VPlanPreheaderVPBB, to emphasize it's VPlan's outer/entry VPBB, to be returned at the end?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:275
   VPBlockBase *HeaderVPBB = getOrCreateVPBB(TheLoop->getHeader());
+  PreheaderVPBB->setOneSuccessor(HeaderVPBB);
   HeaderVPBB->setName("vector.body");
----------------
Is this change needed?

Move the comment about setting Preheader's predecessors along with setOneSuccessor? BTW, comment probably meant to refer to setting the predecessor(s) of Preheader's successor, as Preheader surely has no predecessors set, moreover during RPO traversal below.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:324
 
   // 3. Process outermost loop exit. We created an empty VPBB for the loop
   // single exit BB during the RPO traversal of the loop body but Instructions
----------------
3. >> 2. ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:326
   // single exit BB during the RPO traversal of the loop body but Instructions
   // weren't visited because it's not part of the the loop.
   BasicBlock *LoopExitBB = TheLoop->getUniqueExitBlock();
----------------
createVPInstructionsForVPBB() is removed because exit BB should not be populated; is the current population simply redundant, and can be removed independent of this patch? Should LoopExitVPBB, not being part of the (the) loop, be replaced by LiveOut VPUsers (surely independent of this patch)?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:334
 
-  // 4. The whole CFG has been built at this point so all the input Values must
+  // 4. Fix up region blocks for loops. For each loop,
+  //   * use the header block as entry to the corresponding region,
----------------
4. >> 3. ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:350
+    // Disconnect backedge and pre-header from header.
+    VPBasicBlock *HeaderPred = getOrCreateVPBB(L->getLoopPreheader());
+    VPBlockUtils::disconnectBlocks(HeaderPred, HeaderVPBB);
----------------
HeaderPred >> PreheaderVPBB?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:360
+    // block.
+    VPBasicBlock *LatchSucc = getOrCreateVPBB(L->getExitBlock());
+    VPBlockUtils::disconnectBlocks(LatchVPBB, LatchSucc);
----------------
LatchSucc >> ExitVPBB? Where LatchVPBB == ExitingVPBB.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:368
+  }
+  // 5. The whole CFG has been built at this point so all the input Values must
   // have a VPlan couterpart. Fix VPlan phi nodes by adding their corresponding
----------------
5. >> 4.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:386
+  Plan.setEntry(EntryVPBB);
   LLVM_DEBUG(Plan.setName("HCFGBuilder: Plain CFG\n"); dbgs() << Plan);
 
----------------
Unrelated to this patch: setName under LLVM_DEBUG?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:388
 
+  auto *TopRegion = cast<VPRegionBlock>(EntryVPBB->getSingleSuccessor());
   Verifier.verifyHierarchicalCFG(TopRegion);
----------------
getVectorLoopRegion()?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:27-28
 
-  auto *TopRegion = cast<VPRegionBlock>(Plan->getEntry());
-  ReversePostOrderTraversal<VPBlockBase *> RPOT(TopRegion->getEntry());
-
-  for (VPBlockBase *Base : RPOT) {
-    // Do not widen instructions in pre-header and exit blocks.
-    if (Base->getNumPredecessors() == 0 || Base->getNumSuccessors() == 0)
-      continue;
-
-    VPBasicBlock *VPBB = Base->getEntryBasicBlock();
+  ReversePostOrderTraversal<VPBlockRecursiveTraversalWrapper<VPBlockBase *>>
+      RPOT(VPBlockRecursiveTraversalWrapper<VPBlockBase *>(Plan->getEntry()));
+  for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(RPOT)) {
----------------
Simply
```
  ReversePostOrderTraversal<VPBlockBase *> RPOT(Plan->getEntry());
```
would do, or is the Wrapper needed for blocksOnly()?
Until now it works w/o the blocksOnly<VPBasicBlock>(), because TopRegion is skipped, and there were no other regions.

So now we do widen instructions in pre-header and (empty) exit/middle blocks having zero predecessors and successors respectively, right?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:53
     // Check block's condition bit.
-    if (VPB->getNumSuccessors() > 1)
+    if (VPB->getNumSuccessors() > 1 || Region->getExitBasicBlock() == VPB)
       assert(VPB->getCondBit() && "Missing condition bit!");
----------------
(To be renamed ExitingBasicBlock, which has no successors but a CondBit to denote when to exit the loop...)


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp:49
   EXPECT_EQ(nullptr, Entry->getCondBit());
 
   VPBasicBlock *VecBB = Entry->getSingleSuccessor()->getEntryBasicBlock();
----------------
Can explain:

  // Check that the region following the preheader is a single basic-block region (loop).


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp:177
   EXPECT_EQ(1u, Entry->getNumSuccessors());
 
   VPBasicBlock *VecBB = Entry->getSingleSuccessor()->getEntryBasicBlock();
----------------
ditto, outline to a common function checking for a single basic-block region (loop)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123005