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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

(submit pending comment responses I missed to submit earlier)



================
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>
----------------
Ayal wrote:
> 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.
Thanks, I clarified the comment that this is limited to the iterator traversal.


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


================
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};
----------------
Ayal wrote:
> 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).
> 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?

Added a test in 3632bf85079bc497993def427223e9d74389a659 that use the iterator on a region directly.

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


moved, thanks!

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

done in c95138392a75021c98f53b423b29995cb8a3283b, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140511