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

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9115
       }
     }
   }
----------------
Ayal wrote:
> Another alternative to FillHeaderVPBB above, is to prepare here for the next iteration by doing:
> 
> ```
>   auto *NextVPBB = new VPBasicBlock();
>   VPBlockUtils::insertBlockAfter(NextVPBB, VPBB);
>   VPBB = NextVPBB;
> ```
> The start of each iteration needs to only `VPBB->setName(BB->getName())` which also takes care of the header block; after exiting the loop we can get rid of the last empty VPBB, possibly as part of fusing the latch.
Thanks, updated to use this approach, but with a small extra to avoid generating a redundant empty block in the last iteration.

Alternatively `VPBlockUtils::tryToMergeBlockIntoPredecessor` could be used, if you prefer.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9291
+  }
+
   assert(VPlanVerifier::verifyPlanIsValid(*Plan) && "VPlan is invalid");
----------------
Ayal wrote:
> Would be good to avoid further bloating this excessive method. Could this folding be outlined, or should it be applied only to selected VPlan prior to code-gen rather than every VPlan upon construction? Doesn't VPBasicBlock::execute() effectively fold Exit into its predecessor if possible?
I added a new helper `tryToMergeBlockIntoPredecessor`. 

> . Could this folding be outlined, or should it be applied only to selected VPlan prior to code-gen rather than every VPlan upon construction? Doesn't VPBasicBlock::execute() effectively fold Exit into its predecessor if possible?

The only reason to do this here is to avoid polluting the VPlan printing with additional redundant blocks. We could keep the extra block, but it would require updating all tests that check a printed VPlan and also bloats the plans we need to check in general. WDYT?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2357
   /// has more than one successor, its conditional bit is propagated to \p
   /// NewBlock. \p NewBlock must have neither successors nor predecessors.
   static void insertBlockAfter(VPBlockBase *NewBlock, VPBlockBase *BlockPtr) {
----------------
Ayal wrote:
> Would be good to update above documentation
> - condition bit is propagated? to NewBlock?
> - disconnects BlockPtr from all its successors and connects it with NewBlock as its successor
Should be updated, cond bits are now propagated (and removed from BlockPtr ) and moving successors is mentioned.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2360
     assert(NewBlock->getSuccessors().empty() &&
            "Can't insert new block with successors.");
+    NewBlock->setParent(BlockPtr->getParent());
----------------
Ayal wrote:
> Also assert NewBlock has no predecessors, as documented above?
added, thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115793