[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:1766
+
+  static BlockPtrTy getFirstParentWithSuccs(BlockPtrTy Current) {
+    while (Current && Current->getNumSuccessors() == 0)
----------------
a.elovikov wrote:
> [pedantic]
> Strictly speaking, returning Current isn't returning any parents
> [/pedantic]
> 
> Maybe rename to `getBlockWithSuccs`?
Thanks, I updated the name as suggested.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1772
+
+  template <typename T1, typename T2> static T1 deref(T2 *Ptr) {
+    unsigned SuccIdx = Ptr->SuccessorIdx;
----------------
a.elovikov wrote:
> I don't understand the purpose of `T1` here yet. I'll continue reading assuming
> 
> ```
> template <typename T>
> static auto deref(T *Ptr)
> ```
I think this was needed to make things build, because otherwise Clang failed to determine the correct return type of the function. But I updated the code now to pass the block and index in directly, requiring only one template argument.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100175