[PATCH] D139927: [VPlan] Remove redundant blocks by merging them into predecessors.

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 26 12:16:42 PST 2022


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:328
+      VPBlockUtils::disconnectBlocks(VPBB, Succ);
+      VPBlockUtils::connectBlocks(PredVPBB, Succ);
+    }
----------------
barannikov88 wrote:
> Q1: Can VPBB be a successor of itself? If it can, this is probably fine, as we (1) shouldn't reach here (unless VPBB is unreachable) and (2) reconnecting it to itself should do no harm as it is going to be deleted anyway. Worth asserting?
> Q2: Can VPBB have references other than control flow edges? I'm asking because I see `VPBasicBlock::dropAllReferences` interface.
>. Q1: Can VPBB be a successor of itself? If it can, this is probably fine, as we (1) shouldn't reach here (unless VPBB is unreachable) and (2) reconnecting it to itself should do no harm as it is going to be deleted anyway. Worth asserting?

In theory there's nothing preventing that, but in practice all cycles should be represented as regions without explicit back-edge at the moment.

>  Q2: Can VPBB have references other than control flow edges? I'm asking because I see VPBasicBlock::dropAllReferences interface.

VPBB previously could have other users, but those have been removed AFAICT.

VPBasicBlock::dropAllReferences removes references to all recipes in the block.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:310
+  ReversePostOrderTraversal<VPBlockRecursiveTraversalWrapper<VPBlockBase *>>
+      RPOT(Plan.getEntry());
+  for (VPBlockBase *VPB : RPOT)
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Is RPOT needed (due to destructive / make_early_inc_range) or would depth_first suffice?
> > > 
> > > Suffice to iterate over blocksOnly<VPBasicBlock> and tryToMerge[Basic]BlockIntoPredecessor(VPBasicBlock)?
> > Unforunately `depth_first` doesn't support `make_early_inc_range` at the moment, I am not sure if that's because of VPBlockRecursiveTraversalWrapper or something else.
> > 
> > > Suffice to iterate over blocksOnly<VPBasicBlock> and tryToMerge[Basic]BlockIntoPredecessor(VPBasicBlock)?
> > 
> > Done, thanks!
> Another option may be to scan depth_first and push candidates VPBB's onto a stack that is later traversed (succ before pred) to merge each candidate into its predecessor and delete it. This could potentially minimize RPOT copying overhead, although VPlan sizes are currently small(?)
RPOT is not needed, but if we want to use existing graph iterators I think we need to do the traversal first and then process it.

I updated the code to cache depth-first traversal, to make clear RPOT is not needed. Alternatively we could do the traversal manually, but this would need more work if we want to traverse through regions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139927



More information about the llvm-commits mailing list