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

Mon May 30 12:23:20 PDT 2022

fhahn added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:89
+    auto *PredVPBB = getOrCreateVPBB(Pred);
+    VPBBPreds.push_back(PredVPBB);
+  }
----------------
Ayal wrote:
> Is this change needed?
Nope, removed the stray change, thanks!

================
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) {
----------------
Ayal wrote:
> 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.
Extended the comment, thanks!

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:120

+  // Get or create a region for the loop containing BB.
+  Loop *CurrentLoop = LI->getLoopFor(BB);
----------------
Ayal wrote:
> Deserves a separate getOrCreate[EnclosingLoop]Region()?
Given that is is just used here I'm not sure if it is worth pulling this out. Could be a lambda if you think early exits would help readability?

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:135
   BB2VPBB[BB] = VPBB;
-  VPBB->setParent(TopRegion);
+  VPBB->setParent(ParentR);
   return VPBB;
----------------
Ayal wrote:
> 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.
Good point, looks like we are missing a VPlan printing ttest for outer loop vectorization. Added in b7d2b160c3ba

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

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

It's not needed in the latest version of the patch, changed removed, thanks!

> 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.

Yeah, I'll remove the commit.

================
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
----------------
Ayal wrote:
> 3. >> 2. ?
Numbering updated, thanks!

================
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,
----------------
Ayal wrote:
> 4. >> 3. ?
Numbering updated, thanks!

================
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);
----------------
Ayal wrote:
> HeaderPred >> PreheaderVPBB?
Fixed, thanks!

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:360
+    // block.
+    VPBasicBlock *LatchSucc = getOrCreateVPBB(L->getExitBlock());
+    VPBlockUtils::disconnectBlocks(LatchVPBB, LatchSucc);
----------------
Ayal wrote:
> LatchSucc >> ExitVPBB? Where LatchVPBB == ExitingVPBB.
Done! Also Updated  `LatchVPBB -> ExitingVPBB`, with an assertion that `getLoopLatch() == getExitingBlock()`

================
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
----------------
Ayal wrote:
> 5. >> 4.
Numbering fixed, thanks!

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

----------------
Ayal wrote:
> Unrelated to this patch: setName under LLVM_DEBUG?
Will do separately.

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:388

+  auto *TopRegion = cast<VPRegionBlock>(EntryVPBB->getSingleSuccessor());
   Verifier.verifyHierarchicalCFG(TopRegion);
----------------
Ayal wrote:
> getVectorLoopRegion()?
updated, thanks!

================
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)) {
----------------
Ayal wrote:
> 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?
> Simply
> ReversePostOrderTraversal<VPBlockBase *> RPOT(Plan->getEntry());

Yes, updated, thanks!

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

They will be empty at the moment so nothing happens at the moment.

================
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!");
----------------
Ayal wrote:
> (To be renamed ExitingBasicBlock, which has no successors but a CondBit to denote when to exit the loop...)
Updated!

================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp:49
   EXPECT_EQ(nullptr, Entry->getCondBit());

   VPBasicBlock *VecBB = Entry->getSingleSuccessor()->getEntryBasicBlock();
----------------
Ayal wrote:
> Can explain:
> 
>   // Check that the region following the preheader is a single basic-block region (loop).
Added, thanks!

================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp:177
   EXPECT_EQ(1u, Entry->getNumSuccessors());

   VPBasicBlock *VecBB = Entry->getSingleSuccessor()->getEntryBasicBlock();
----------------
Ayal wrote:
> ditto, outline to a common function checking for a single basic-block region (loop)?
Added, thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123005