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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1753
+template <typename BlockTy>
+class VPTraversingIterator
+    : public iterator_facade_base<VPTraversingIterator<BlockTy>,
----------------
a.elovikov wrote:
> 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.
I changed the name to `VPAllSuccessorsIterator`. WDY?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1756
+                                  std::forward_iterator_tag, VPBlockBase> {
+  BlockTy Block;
+  size_t Idx;
----------------
a.elovikov wrote:
> I'd probably spell it as
> 
>   `BlockTy const Block`
> 
> or
> 
>   `BlockTy * const Block`
Unfortunately it looks like the RPOT helpers uses `std::copy`, which requires the assignment operator, which is not compatible with making `Block` const, but I may be missing some workaround?


================
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) {
----------------
a.elovikov wrote:
> 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`).
Yep, that's not handled properly at the moment! I also realized that the case where the region is the last block in the graph was not handled properly, as we would not enter the region itself. Both issues should be fixed now.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1799
+
+  BlockTy operator*() {
+    // For exit blocks, use the successors of their parent region.
----------------
a.elovikov wrote:
> I'd prefer to use `const_cast` magic here, instead of duplicating the code.
Using `const_cast` did not seem to work well with the graph traits, so I added a templated version. WDYT?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1866
+  static NodeRef
+  getEntryNode(VPBlockRecursiveTraversalWrapper<const VPBlockBase *> N) {
+    return N.getEntry();
----------------
a.elovikov wrote:
> Would it be possible to pass just `const VPBlockBase *` here? Is the argument here what is usually defined as `using GraphType = <something>`?
I think this needs to match the type `GraphTraits` is defined for. Otherwise this cannot be used directly with the wrapper object and it may cause additional problems because there's a `GraphTraits` specialization for `const VPBlockBase *`.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100175