[PATCH] D140511: [VPlan] Disconnect VPRegionBlock from successors in graph iterator(NFCI)

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:148
+// traversal of the graph. Region blocks themselves are not directly connected
+// to their successors.
 template <typename BlockPtrTy>
----------------
nit: the Region blocks themselves are (continue to be) directly connected to their successors, it is how the iterate traverses successors of a region - as those of its exit blocks, only.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:159
   size_t SuccessorIdx;
 
   static BlockPtrTy getBlockWithSuccs(BlockPtrTy Current) {
----------------
nit: worth commenting that null is returned if given block and all its parents have no successors. (Independent of this patch.)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:197
     if (auto *R = dyn_cast<VPRegionBlock>(Block))
-      return {R, NumSuccessors + 1};
+      return {R, 1};
     return {Block, NumSuccessors};
----------------
This is fine as every region has an entry block, which is what this 1 represents, independent of how many successors R may have, if any.

So current VPAllSuccessorIterator visits successors of a region twice - once coming from the exit block and once coming from the region, and the latter is removed here? Is it possible to test that it iterates over blocks correctly?

nit: can define NumSuccessors after early exiting if Block is region, or set it to 1 for regions instead of early exiting.

nit: can set NumSuccessors to zero if !ParentWithSuccs, instead of Block->getNumSuccessors(). (Independent of this patch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140511