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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks fine! Adding a couple of nits and future thoughts:

- refactor recipes that reset a builder to getPreHeaderBBFor(), placing newly introduced recipes in the preheader VPBB instead?
- refactor VPlan::prepareToExecute() into preheader recipes?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3099
+  // Update dominator for loop exit. During skeleton creation, only the vector
+  // pre-heaader and the middle block are created. The vector loop is entirely
+  // created during VPlan exection.
----------------
typo: heaader 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8749
+
   VPBasicBlock *HeaderVPBB = new VPBasicBlock();
   VPBasicBlock *LatchVPBB = new VPBasicBlock("vector.latch");
----------------
(nit: could we initialize HeaderVPBB's name to "vector.body" here, next to vector.latch and vector.ph, instead of overwriting it below, provided we guard setting VPBB's name to BB->getName() if VPBB != HeaderVPBB?)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:343
         PrevVPBB->getSingleHierarchicalSuccessor() &&
-        (SingleHPred->getParent() == GetEnclosingNonReplicateRegion(this) &&
+        (SingleHPred->getParent() == getEnclosingLoopRegion() &&
          !IsNonReplicateR(SingleHPred))) &&      /* B */
----------------
getEnclosingLoopRegion() assumes/asserts we're in a loop; perhaps have it return null if we're not?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:431
+    P = P->getParent();
+    assert(!cast<VPRegionBlock>(P)->isReplicator());
+  }
----------------
Add error message to the assert.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:938
 /// Generate the code inside the body of the vectorized loop. Assumes a single
 /// LoopVectorBody basic-block was created for this. Introduce additional
 /// basic-blocks as needed, and fill them all.
----------------
Update comment.
"inside the body" >> "inside the preheader and body"
"Assumes a single LoopVectorBody basic-block was created for this" - a single preheader basic-block?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1032
 
+  BasicBlock *VectorHeaderBB = State->CFG.VPBB2IRBB[Header];
   // We do not attempt to preserve DT for outer loop vectorization currently.
----------------
place below inside the 'if'?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1481
+
+  BasicBlock *VectorPH = State.CFG.getPreheaderBBFor(this);
+  Value *Res =
----------------
Do we need to getPreheaderBBFor(this), given that the recipe should reside in the preheader VPBB?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121624