[PATCH] D70734: [VPlan] Add basicblocks() and loop_basicblocks iterators.

Sat Dec 7 01:43:53 PST 2019

gilr added a comment.

In D70734#1765693 <https://reviews.llvm.org/D70734#1765693>, @fhahn wrote:

> In D70734#1764103 <https://reviews.llvm.org/D70734#1764103>, @gilr wrote:
>
> > > Traversing VPRegions is not supported at the moment but we do not create such plans at the moment as far as I know
> >
> > I assume you're referring to the native path? (non-native generates regions for scalarized & predicated instructions)
>
>
> Ah right, I missed that code path! For my initial use-case (dropping assumes), the iterator needs to traverse the regions as well then. Let's see if I can get the iterators to co-operate. Do you think it's worth to get the existing iterators in as is? There are a few places that could use them, I think.

Probably best to bundle it with at least one concrete use, to avoid potential bit rot.

================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1404
+  /// the loop blocks.
+  iterator_range<vpbb_iterator_adaptor> basicblocks() {
+    VPRegionBlock *TopRegion = dyn_cast<VPRegionBlock>(Entry);
----------------
fhahn wrote:
> gilr wrote:
> > basicBlocks()?
> I am not sure. I think for most iterator ranges we use lower case naming, with underscores, but I am not sure why. Personally, I think `basicBlocks()` looks a bit odd. Same for `loopBasicBlocks`. But I'd happily change the name, if there's a strong preference.
>  I think for most iterator ranges we use lower case naming, with underscores

ah, then please keep the current names.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70734