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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:52
+  // Map incoming BasicBlocks to their newly-created VPBasicBlocks and
+  // VPRegionBlocks.
+  DenseMap<BasicBlock *, VPBlockBase *> BB2VPB;
----------------
Ayal wrote:
> 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?
Undone changes here, `getOrCreateVPB` checks if the created VPBB is the entry for a region. I still retained `getOrCreateVPB`, because it is convenient to get the block to set the successors, e.g. when connecting the pre-header to the inner loop header.


================
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) {
+
----------------
Ayal wrote:
> Would two methods be clearer, one taking a VPBasicBlock and the other a VPRegionBlock?
Moved out, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:93
+  if (isa<VPRegionBlock>(VPBB)) {
+    for (BasicBlock *Pred : predecessors(BB)) {
+      auto *PredVPBB = getOrCreateVPBB(Pred);
----------------
Ayal wrote:
> Can be simplified - trying to select one of two expected predecessors?
Retained loop to add an assert that we only have. single predecessors.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:106
+    auto *PredVPBB = getOrCreateVPBB(Pred);
+    if (PredVPBB->getParent() == VPBB->getParent())
+      VPBBPreds.push_back(PredVPBB);
----------------
Ayal wrote:
> Should avoid connecting VPBBPred-VPBB that are latch-header, handled above?
Yes, the check here is to catch setting the predecessor for the exit block, as it needs to connect to the predecessor region. Added a comment.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:108
+      VPBBPreds.push_back(PredVPBB);
+    else
+      VPBBPreds.push_back(PredVPBB->getParent());
----------------
Ayal wrote:
> 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?
In the else case, VPBB Is the exit block of PredVPBB's region, which isn't handled elsewhere (checking elsewhere would be more complicated I think.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:128
       VPPhi->addIncoming(getOrCreateVPOperand(Phi->getIncomingValue(I)),
-                         BB2VPBB[Phi->getIncomingBlock(I)]);
+                         getOrCreateVPBB(Phi->getIncomingBlock(I)));
   }
----------------
Ayal wrote:
> Independent cleanup?
This was needed after changing the map, undone now.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:314
+  VPBlockBase *HeaderVPBB = getOrCreateVPB(TheLoop->getHeader());
+  getOrCreateVPBB(TheLoop->getHeader())->setName("vector.body");
   ThePreheaderVPBB->setOneSuccessor(HeaderVPBB);
----------------
Ayal wrote:
> Why/Is the above change needed? HeaderVPBB sounds like a VPBasicBlock, rather than its region.
We need to connect the pre-header to the top region and set the name of the header block (to avoid unnecessary test changes). Update to clarify that.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:336
       assert(SuccVPBB && "VPBB Successor not found.");
       VPBB->setOneSuccessor(SuccVPBB);
     } else if (NumSuccs == 2) {
----------------
Ayal wrote:
> nit: assert VPBB and SuccVPBB have same parent.
> 
> nit: set predecessors (checking if header?) and early continue;
Added assert to `setOneSuccessor` and fixed an issue uncovered where the parent for regions wasn't set properly.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:349
              "Missing condition bit in IRDef2VPValue!");
 
+      VPRegionBlock *Region = VPBB->getParent();
----------------
Ayal wrote:
> 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).
Updated to handle header and latch separately while setting successors. To simplify the latter, I updated the code to collect the successors in a vector, as this is used to check for the latch.


================
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) {
----------------
Ayal wrote:
> SuccVPBB >> ExitVPBB, or OutOfRegionSuccVPBB.
adjusted, thanks!


================
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);
----------------
Ayal wrote:
> Better to check if BB is a header before calling setVPBBPredsFromBB?
Updated and moved before setting successors. Checking if it's a header can be done by checking if the VPBB is the entry to its parent region


================
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();
----------------
Ayal wrote:
> 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.
Yes, predecessors are only set once the block has been visited in RPO for that reason, so it needs to be done explicitly 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.
----------------
Ayal wrote:
> nit: independent cleanup?
Was only needed after changing the map, undone, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159136