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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Patch is ready to go in, with a comment explaining why the empty pre-latch VPBB is merged early and the latch VPBB is merged late.

Suggestions to merge both blocks early, and other comments added inline, may be further discussed in follow-up patch(es) if preferred, or in this one.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9116
+    VPBlockUtils::insertBlockAfter(NextVPBBForBB, VPBB);
+    VPBB = NextVPBBForBB;
   }
----------------
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)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9124
+  (void)Folded;
+  VPBB = PrevVPBB;
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9196
       if (VPBB == SplitPred)
         VPBB = SplitBlock;
     }
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9208
   // Adjust the recipes for any inloop reductions.
   adjustRecipesForReductions(VPBB, Plan, RecipeBuilder, Range.Start);
 
----------------
The use of VPBB here suggests that Exit be used instead, but does it use/rely on a BB preceding the latch?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9290
+  VPBlockUtils::tryToMergeBlockIntoPredecessor(LatchVPBB);
+
   assert(VPlanVerifier::verifyPlanIsValid(*Plan) && "VPlan is invalid");
----------------
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().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115793