[PATCH] D115793: [VPlan] Create header & latch blocks for plan skeleton up front (NFC).

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9116
+    VPBlockUtils::insertBlockAfter(NextVPBBForBB, VPBB);
+    VPBB = NextVPBBForBB;
   }
----------------
Ayal wrote:
> Can alternatively do:
> 
> ```
>     VPBlockUtils::insertBlockAfter(new VPBasicBlock(), VPBB);
>     VPBB = VPBB->getSingleSuccessor();
> ```
> or have insertBlockAfter() return the block it inserted and have
> `    VPBB = VPBlockUtils::insertBlockAfter(new VPBasicBlock(), VPBB);`
> (in this or separate patch)
Updated to use the first alternative, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9124
+  (void)Folded;
+  VPBB = PrevVPBB;
+
----------------
Ayal wrote:
> Perhaps tryToMergeBlockIntoPredecessor() should return the predecessor it merged into if successful (null otherwise), to support VPBB = VPBlockUtils::tryToMergeBlockIntoPredecessor(VPBB);
> 
> But probably better to avoid updating VPBB at all (tryToMerge already takes care of updating Exit if needed) - see below.
> But probably better to avoid updating VPBB at all (tryToMerge already takes care of updating Exit if needed) - see below.

Sounds good! The only real later use was `adjustRecipesForReductions`.  It expects the latch block, so `LatchVPBB` can be passed instead. This required a fix to ensure that `VPWidenCanonicalIVRecipe` is always inserted in the header: 1a54889f48fa


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9196
       if (VPBB == SplitPred)
         VPBB = SplitBlock;
     }
----------------
Ayal wrote:
> This updating of VPBB is essentially trying to maintain the last BB, i.e., the Latch, i.e., Exit. Would be good to use VPBB only during initial VPlan construction, and refer to Exit instead of VPBB afterwards when seeking the latch.
Removed!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9208
   // Adjust the recipes for any inloop reductions.
   adjustRecipesForReductions(VPBB, Plan, RecipeBuilder, Range.Start);
 
----------------
Ayal wrote:
> The use of VPBB here suggests that Exit be used instead, but does it use/rely on a BB preceding the latch?
`adjustRecipesForReductions` expects the latch block (the argument is named accordingly), so `LatchVPBB` can be used directly..


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9290
+  VPBlockUtils::tryToMergeBlockIntoPredecessor(LatchVPBB);
+
   assert(VPlanVerifier::verifyPlanIsValid(*Plan) && "VPlan is invalid");
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Should this folding of the latch be done right after folding the empty last VPBB above, or better do so here after dumping Plan.
> > I don't think so. I think main motivation is to have the latch/exit block separately so sink-after & other transforms have do not have to explicitly updated the exit block.
> There are two motivations: (1) set the Exit when its region is created, (2) designate a unique BB for the Exit.
> The first provides a stable region, allowing recipes to be placed in the latch during construction if needed, and resolves the issue of when to set Exit; we know a region must have an Exit, may as well have one at the outset.
> The second facilitates introducing blocks internal to the loop, between header and latch, requiring only a disconnect and reconnects rather than splitting a block. This is most useful during initial VPlan construction, but seems useless for later transformations - which in general may need to split blocks.
> 
> Note that sink-after (splits blocks and) already explicitly updates the "last" BB, see comment above, so may as well have it update the Exit block instead. Would be good to have splitAt() update the enclosing Region's Exit if needed, as well, similar to tryToMergeBlockIntoPredecessor().
> Note that sink-after (splits blocks and) already explicitly updates the "last" BB, see comment above, so may as well have it update the Exit block instead. Would be good to have splitAt() update the enclosing Region's Exit if needed, as well, similar to tryToMergeBlockIntoPredecessor().

Sounds like a good follow-up. I'd prefer to keep the folding late for now, as there is a verifier error when it is folded earlier and I'd prefer to fix that separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115793