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

[Andrei Elovikov via Phabricator via llvm-commits]

a.elovikov added a comment.

I think it's functionally correct now, just have a few suggestions to simplify the code a little bit.

Thanks for the updates!



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1766
+
+  static BlockPtrTy getFirstParentWithSuccs(BlockPtrTy Current) {
+    while (Current && Current->getNumSuccessors() == 0)
----------------
[pedantic]
Strictly speaking, returning Current isn't returning any parents
[/pedantic]

Maybe rename to `getBlockWithSuccs`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1772
+
+  template <typename T1, typename T2> static T1 deref(T2 *Ptr) {
+    unsigned SuccIdx = Ptr->SuccessorIdx;
----------------
I don't understand the purpose of `T1` here yet. I'll continue reading assuming

```
template <typename T>
static auto deref(T *Ptr)
```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1774
+    unsigned SuccIdx = Ptr->SuccessorIdx;
+    if (auto *R = dyn_cast<VPRegionBlock>(Ptr->Block)) {
+      if (SuccIdx == 0)
----------------
nit: Ptr->Block is used three times, might be beneficial to outline to a variable with explicit type (VPBlockBase?)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1783
+    if (Ptr->Block->getNumSuccessors() == 0 && ParentWithSuccs)
+      return ParentWithSuccs->getSuccessors()[SuccIdx];
+
----------------
I think doing this unconditionally would just work. The current implementation of `getFirstParentWithSuccs` returns `Ptr->block` if it has successors.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1802
+    BlockPtrTy ParentWithSuccs = getFirstParentWithSuccs(Block);
+    unsigned NumSuccessors = Block->getNumSuccessors() == 0 && ParentWithSuccs
+                                 ? ParentWithSuccs->getNumSuccessors()
----------------
I think it should be just `ParentWithSuccs ? ParentWithSuccs->getNumSuccessors() : 0` after implementing the suggestion above about renaming the method.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1799
+
+  BlockTy operator*() {
+    // For exit blocks, use the successors of their parent region.
----------------
fhahn wrote:
> 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?
That would work, but please add the comment near the templated version what this template paramter is used for (to support const/non-const versions). I assume it would be the `deref` function, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100175