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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:310
+  ReversePostOrderTraversal<VPBlockRecursiveTraversalWrapper<VPBlockBase *>>
+      RPOT(Plan.getEntry());
+  for (VPBlockBase *VPB : RPOT)
----------------
fhahn wrote:
> 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.
Ahh, right, candidates can also be visited in pred-before-succ order, or any other.

Original thought was to collect all VPBB's having a single BB predecessor which in turn has a single successor, as this collection is expected to be smaller than all blocks/basic-blocks, if not empty; plus one could return !empty() instead of `Changed`. But this is mostly a matter of style, i.e., separating the logic into inspection and execution. Current approach is also fine, what would you prefer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139927