[PATCH] D100175: [VPlan] Add GraphTraits impl to traverse through VPRegionBlock.

[Andrei Elovikov via Phabricator via llvm-commits]

a.elovikov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1753
+template <typename BlockTy>
+class VPTraversingIterator
+    : public iterator_facade_base<VPTraversingIterator<BlockTy>,
----------------
IMO, having a work "successors" in the name would help in understanding the logic behind this. I've caught myself on a though that at some point reading the code below I've forgotten that we only go through the "successors" of a a block and not through the whole graph.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1756
+                                  std::forward_iterator_tag, VPBlockBase> {
+  BlockTy Block;
+  size_t Idx;
----------------
I'd probably spell it as

  `BlockTy const Block`

or

  `BlockTy * const Block`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1757
+  BlockTy Block;
+  size_t Idx;
+
----------------
Can we rename it to `SuccessorIdx`? Do I understand the purpose of the field correctly?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1785
+  const VPBlockBase *operator*() const {
+    // For exit blocks, use the successors of their parent region.
+    if (Block->getParent() && Block->getNumSuccessors() == 0) {
----------------
What if the block's parent is a region that is an exit as well? Shouldn't we recursively go up the chain of such exits? Same throughout other methods (e.g. `end`).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1799
+
+  BlockTy operator*() {
+    // For exit blocks, use the successors of their parent region.
----------------
I'd prefer to use `const_cast` magic here, instead of duplicating the code.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1828
+/// Helper for GraphTraits specialization that traverses through VPRegionBlocks.
+template <typename BlockTy> class VPBlockRecursiveTraversalWrapper {
+  BlockTy Entry;
----------------
Wow, that is nice. I was only aware of the approach with named traits and then defining helper function to pass that trait explicitly to traversal. Your approach is much clearer.


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:346
+    SmallVector<const VPBlockBase *> FromIterator;
+    for (const VPBlockBase *VPBB : depth_first(
+             VPBlockRecursiveTraversalWrapper<const VPBlockBase *>(VPBB1)))
----------------
nit:

  llvm::copy(depth_first(...), std::back_inserter(FromIterator));


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:380
+    VPBlockUtils::connectBlocks(R1, R2);
+
+    VPBlockRecursiveTraversalWrapper<VPBlockBase *> N = {R1};
----------------
Can you please add an ASCII graph drawing here similar to the one above?


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:434
+  //   R2 {      |
+  //     R2BB1   |
+  //       |    R1BB2
----------------
Can we either modify this test or add another one with some non-linear CFG inside R2?


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:467
+
+    SmallVector<VPBlockBase *> FromIterator(
+        df_begin(VPBlockRecursiveTraversalWrapper<VPBlockBase *>(VPBB1)),
----------------
nit: I think `llvm::copy` or outlining the region into a temporary `auto` variable would help with readability here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100175