[PATCH] D159136: [VPlan] Update successors/predecessors of region blocks directly (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Title should indicate this is an HCFG simplification patch.

Worth indicating that this refactoring assists D158333 <https://reviews.llvm.org/D158333>.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:52
+  // Map incoming BasicBlocks to their newly-created VPBasicBlocks and
+  // VPRegionBlocks.
+  DenseMap<BasicBlock *, VPBlockBase *> BB2VPB;
----------------
Mapping BB's to their VPBB's except for header BB's that are mapped to the enclosing VPRegion may be confusing (exacerbated by reusing VPBB or using VPB for VPBlockBase), and should be documented.

Would it be better to keep BB2VPBB as is, mapping BB's to their VPBasicBlocks, and augment it with a getRegionOfHeader() function that checks if a VPBB is the entry of its enclosing region and if so returns the region, otherwise returns the given VPBasicBlock?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:85
 // must have no predecessors.
-void PlainCFGBuilder::setVPBBPredsFromBB(VPBasicBlock *VPBB, BasicBlock *BB) {
+void PlainCFGBuilder::setVPBBPredsFromBB(VPBlockBase *VPBB, BasicBlock *BB) {
+
----------------
Would two methods be clearer, one taking a VPBasicBlock and the other a VPRegionBlock?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:93
+  if (isa<VPRegionBlock>(VPBB)) {
+    for (BasicBlock *Pred : predecessors(BB)) {
+      auto *PredVPBB = getOrCreateVPBB(Pred);
----------------
Can be simplified - trying to select one of two expected predecessors?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:106
+    auto *PredVPBB = getOrCreateVPBB(Pred);
+    if (PredVPBB->getParent() == VPBB->getParent())
+      VPBBPreds.push_back(PredVPBB);
----------------
Should avoid connecting VPBBPred-VPBB that are latch-header, handled above?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:108
+      VPBBPreds.push_back(PredVPBB);
+    else
+      VPBBPreds.push_back(PredVPBB->getParent());
----------------
Two cases:
1. VPBBPred's parent == VPBB's grandparent: VPBBPred-VPBB are preheader-header, handled above?
2. VPBBPred's grandparent == VPBB's parent: VPBBPred-VPBB are latch-exit, handled here?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:128
       VPPhi->addIncoming(getOrCreateVPOperand(Phi->getIncomingValue(I)),
-                         BB2VPBB[Phi->getIncomingBlock(I)]);
+                         getOrCreateVPBB(Phi->getIncomingBlock(I)));
   }
----------------
Independent cleanup?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:134
+  auto BlockIt = BB2VPB.find(BB);
+  if (BlockIt != BB2VPB.end())
+    // Retrieve existing VPB.
----------------
nit: deserves brackets.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:153
   VPBB->setParent(ParentR);
+  if (CurrentLoop && CurrentLoop->getHeader() == BB) {
+    BB2VPB[BB] = ParentR;
----------------
This check if BB is a header should probably be generalized (e.g., into isHeader(BasicBlock*)) and used below.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:314
+  VPBlockBase *HeaderVPBB = getOrCreateVPB(TheLoop->getHeader());
+  getOrCreateVPBB(TheLoop->getHeader())->setName("vector.body");
   ThePreheaderVPBB->setOneSuccessor(HeaderVPBB);
----------------
Why/Is the above change needed? HeaderVPBB sounds like a VPBasicBlock, rather than its region.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:328
     // exist already. Recipes will be created when the successor is visited
     // during the RPO traversal.
     Instruction *TI = BB->getTerminator();
----------------
nit: "during the RPO traversal" - Recipes will be created for each successor when it is reached later during this RPO traversal.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:336
       assert(SuccVPBB && "VPBB Successor not found.");
       VPBB->setOneSuccessor(SuccVPBB);
     } else if (NumSuccs == 2) {
----------------
nit: assert VPBB and SuccVPBB have same parent.

nit: set predecessors (checking if header?) and early continue;


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:349
              "Missing condition bit in IRDef2VPValue!");
 
+      VPRegionBlock *Region = VPBB->getParent();
----------------
Worth dealing here with four cases explicitly: BB is header or not, BB is latch or not?

Setting predecessors: easier to check if BB is header of its loop, and if so hook the region of its VPBB (setting its entry) to a single predecessor - the VPBB of preheader - having a distinct Region? Otherwise setVPBBPredsFromBB.

Setting successors: easier to check if BB is latch of its loop, and if so hook the region of its VPBB (setting its exit) to a single successor - the VPBB of exit - having a distinct Region? Otherwise set VPBB's successor(s).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:355
+      if (Region && (SuccVPBB0 == Region || SuccVPBB1 == Region)) {
+        VPBlockBase *SuccVPBB = SuccVPBB0 == Region ? SuccVPBB1 : SuccVPBB0;
+        if (Region->getNumSuccessors() == 0) {
----------------
SuccVPBB >> ExitVPBB, or OutOfRegionSuccVPBB.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:367
     } else
       llvm_unreachable("Number of successors not supported.");
     // Set VPBB predecessors in the same order as they are in the incoming BB.
----------------
nit: consider asserting at the outset that NumSuccs is 1 or 2, to save indentation.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:368
       llvm_unreachable("Number of successors not supported.");
-
     // Set VPBB predecessors in the same order as they are in the incoming BB.
+    setVPBBPredsFromBB(getOrCreateVPB(BB), BB);
----------------
Better to check if BB is a header before calling setVPBBPredsFromBB?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:374
   // 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();
----------------
An empty VPBB was created, and set as successor, but not hooked up to its predecessor, which is done here. Setting predecessors separately from setting successors (rather than together as bidirectional edges), presumably to keep predecessors in order.
Instructions of loop exit weren't visited, nor are they visited here.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:377
   assert(LoopExitBB && "Loops with multiple exits are not supported.");
-  VPBasicBlock *LoopExitVPBB = BB2VPBB[LoopExitBB];
+  VPBasicBlock *LoopExitVPBB = getOrCreateVPBB(LoopExitBB);
   // Loop exit was already set as successor of the loop exiting BB.
----------------
nit: independent cleanup?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159136