[PATCH] D121624: [VPlan] Model pre-header explicitly.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8709
 
   // Create initial VPlan skeleton, with separate header and latch blocks.
   VPBasicBlock *HeaderVPBB = new VPBasicBlock();
----------------
Comment deserves updating.

Start by creating Preheader, then Plan, and later insert TopRegion after Preheader?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9491
+                              ->getSingleHierarchicalPredecessor()
+                              ->getExitBasicBlock()];
+  VecInd->addIncoming(SteppedStart, VectorPreHeader);
----------------
Worth caching in State.CFG.VectorPreHeader?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:309
   // The last IR basic block is reused, as an optimization, in three cases:
   // A. the first VPBB reuses the loop header BB - when PrevVPBB is null;
+  //   A2. the current VPBB is not the entry block of the vector loop region
----------------
"the loop header BB" >> "the loop preheader BB" ?

A2 should be a refinement of B: fold {single succ, single pred} pairs, provided they do not belong to distinct rotating regions?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:468
+      State->LI->addTopLevelLoop(State->CurrentVectorLoop);
+    }
+
----------------
nit: brackets can be removed


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:475
         // check with the check for loop exit.
         if (Block->getNumSuccessors() == 0)
           continue;
----------------
nit: if's can be fused


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:921
 
   // Remove the edge between Header and Latch to allow other connections.
   // Temporarily terminate with unreachable until CFG is rewired.
----------------
"between Header and Latch" >> between PreHeader and its successor/Header ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:931
 
   // 1. Generate code in loop body.
   State->CFG.PrevVPBB = nullptr;
----------------
"loop body" >> "loop preheader and body"


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:77
 
   // Build the plain CFG and return its Top Region.
+  VPBasicBlock *buildPlainCFG();
----------------
Can continue to return its Top Region, with the understanding that it succeeds a newly introduced preheader VPBB.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:83
 // Set predecessors of \p VPBB in the same order as they are in \p BB. \p VPBB
 // must have no predecessors.
+void PlainCFGBuilder::setVPBBPredsFromBB(VPBasicBlock *VPBB, BasicBlock *BB,
----------------
Comment that/why \p HeaderBB is excluded?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:259
   // Loop PH needs to be explicitly visited since it's not taken into account by
   // LoopBlocksDFS.
   BasicBlock *PreheaderBB = TheLoop->getLoopPreheader();
----------------
Still the case?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:309
              "Missing condition bit in IRDef2VPValue!");
-      VPValue *VPCondBit = IRDef2VPValue[BrCond];
 
+      if (TI->getSuccessor(0) == TheLoop->getHeader())
----------------
Explain that we avoid representing backedges from latch to header blocks - a loop is instead represented by a parenting region?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:344
 
   // 5. Final Top Region setup. Set outermost loop pre-header and single exit as
   // Top Region entry and exit.
----------------
update comment


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.h:58
 
   /// Build plain CFG for TheLoop. Return a new VPRegionBlock (TopRegion)
   /// enclosing the plain CFG.
----------------
Update comment.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:30
   for (VPBlockBase *Base : RPOT) {
     // Do not widen instructions in pre-header and exit blocks.
+    if (Base->getNumSuccessors() == 0)
----------------
update comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121624