[PATCH] D150398: [VPlan] Model branch cond to enter scalar epilogue in VPlan.

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3081
+  // FIXME: Model exit block and scalar preheader directly in VPlan to avoid
+  // creating initial branch here and fixing it up during VPlan execution.
   BranchInst *BrInst =
----------------
Ayal wrote:
> Can this FIXME be addressed so that VPlan execution will take care of setting the successors of this branch, naturally?
> 
> In addition to moving the generation of ICmpEQ from ILV::completeLoopSkeleton() to a VPInstruction introduced in VPlan::createInitialVPlan(), can the generation of LoopMiddleBlock and LoopScalarPreHeader block move from ILV::createVectorLoopSkeleton() to VPlan? Can both be applied as a VPlan transform, perhaps alongside optimizeForVFAndUF() - knowing VF*UF may help determine if it divides TC and thus middle block should jump to exit unconditionally (or have loop latch jump to exit directly). ILV will then detect the middle block and scalar loop pre-header built by VPlan and set its LoopMiddleBlock and LoopScalarPreHeader variables.
Yes I think so, but it will possibly require some extra logic to update the dominator tree. In terms of ordering of changes, I think we would need to first move out the branch to be modeled in VPlan, before moving the block creating itself.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7659
   if (!BestVPlan.getPreheader()->empty()) {
     State.CFG.PrevBB = OrigLoop->getLoopPreheader();
     State.Builder.SetInsertPoint(OrigLoop->getLoopPreheader()->getTerminator());
----------------
Ayal wrote:
> A VPBB which is invariantly associated with an existing IRBB should perhaps record this association when created, rather than via State.CFG.PrevBB/ExitBB. This includes the (pre-)pre-header, exit block, and potentially original loop blocks from header to latch. Such VPBB's facilitate hooking VPlan's boundary to existing IR, injecting recipes into existing blocks, reusing original loop for default and/or remainder loops, and as a basis to clone and optimize it. Such VPBB's may contain an "in-place" recipe that represents the existing IR Instructions (w/o generating them/anything during execute()), to model their live-ins and live-outs, and as a placeholder for injecting other recipes before/after. The existence of such a recipe implies that its VPBB is "in-place", so the associated IRBB could be retrieved from it.
PrevBB is used during execution to figure out if blocks can be re-used (mostly around replicate regions now we have VPlan based merging). 

For the others, recording the association explicitly sounds good, but maybe even better to try to replace their uses by moving the logic to VPlan explicitly (e.g how to connect them). 

An `in-place` recipe could also help gradually moving parts of the skeleton into VPlan.  IIUC this is mostly a. suggestion for future improvements correct?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8876
+  //    Thus if tail is to be folded, we know we don't need to run the
+  //    remainder and we can use the previous value for the condition (true).
+  // 3) Otherwise, construct a runtime check.
----------------
Ayal wrote:
> Here 2) conceptually includes both "tail is to be folded" and "there is no tail", i.e., when VF*UF is known to divide TC.
> Can start with one "canonical" form, for arbitrary VF,UF and un/folded tail, and later revise VPlan when fixing VF,UF and (un)folding tail.
> 
> The three cases of (1) jump to scalar preheader, (2) jump to exit, (3) jump to either scalar preheader or exit, are compressed here into a single boolean RequiresScalarEpilogueCheck?
This initially tries to just follow the existing logic, but can be broken down further in future patches.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:405
 
   // Hook up the new basic block to its predecessors.
   for (VPBlockBase *PredVPBlock : getHierarchicalPredecessors()) {
----------------
Ayal wrote:
> Could the following logic of hooking up forwards branches when executing their target basic blocks be employed to backpatch the successors of the Middle block's branch targeting the exit block and/or scalar preheader? Or rather the complementary logic in VPBB::execute for IR BB's that are reused (and rewire branches to them there) instead of created here.
Hmm, one issue is that we would need to rely on the IR to figure that out, which means we need to rely on the branch in the middle block implicitly here. The back-patching shouldn't be needed once the successors are also modeled in VPlan (and middle-block is created directly in VPlan). WDYT?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:595
+  return (ParentRegion && ParentRegion->getExitingBasicBlock() == this) ||
+         (!ParentRegion && getNumSuccessors() == 0);
 }
----------------
Ayal wrote:
> The term Exiting block typically means the first case only, i.e., a block inside a loop which can exit the loop (to an Exit block). The proposed change effectively checks if this VPBB has no successors?
I think this check is currently only used to check if the block is supposed to have a conditional terminator, so maybe the function should be renamed to reflect that?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1000
     ICmpULE,
+    ICmpEQ,
     SLPLoad,
----------------
Ayal wrote:
> nit: try to keep in lex order?
> 
> Should the various condition bit Predicates be provided by VPRecipeWithIRFlags as yet another set of flags, having a derived VPInstructions with `ICmp` and `FCmp` opcodes?
Yes I have been thinking something along the same lines. It might stretch the `IRFlags` a bit but should work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150398