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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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();
----------------
Ayal wrote:
> Comment deserves updating.
> 
> Start by creating Preheader, then Plan, and later insert TopRegion after Preheader?
Added comment with up-to-date info about the initial skeleton.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9491
+                              ->getSingleHierarchicalPredecessor()
+                              ->getExitBasicBlock()];
+  VecInd->addIncoming(SteppedStart, VectorPreHeader);
----------------
Ayal wrote:
> Worth caching in State.CFG.VectorPreHeader?
I moved the logic to `VPTransformState::CFGState::getPreheaderBBFor`. Not sure about caching it, as it may require a bit more tracking/invalidation for nested vector-loop regions with pre-headers, whereas the current code should handle them properly.


================
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
----------------
Ayal wrote:
> "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?
> "the loop header BB" >> "the loop preheader BB" ?

Done, thanks!

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

Sounds good, I adjusted the code to require blocks to share the same (non-replicate) region.


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


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:77
 
   // Build the plain CFG and return its Top Region.
+  VPBasicBlock *buildPlainCFG();
----------------
Ayal wrote:
> Can continue to return its Top Region, with the understanding that it succeeds a newly introduced preheader VPBB.
`buildPlainCFG` also creates the pre-header, so it seems more direct to return it directly, rather than requiring the callers to possibly get the predecessor?

I updated the comment but kept the return value the same.


================
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,
----------------
Ayal wrote:
> Comment that/why \p HeaderBB is excluded?
Added


================
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();
----------------
Ayal wrote:
> Still the case?
Yes I think so. Nothing should have changed with respect to LoopBlockDFS not visiting the pre-header.


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


================
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.
----------------
Ayal wrote:
> update comment
Updated, thanks!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121624