Page MenuHomePhabricator

[VPlan] Add basicblocks() and loop_basicblocks iterators.
Needs ReviewPublic

Authored by fhahn on Nov 26 2019, 11:14 AM.

Details

Reviewers
hsaito
Ayal
gilr
Summary

This patch adds 2 convenience iterator ranges, to iterate over all
VPBasicBlocks in a plan. Traversing VPRegions is not supported at the
moment, but we do not create such plans at the moment as far as I know.
They come with an assertion, to implement the additional functionality
once required.

Diff Detail

Event Timeline

fhahn created this revision.Nov 26 2019, 11:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2019, 11:14 AM
gilr added a comment.Nov 29 2019, 10:32 AM

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)

llvm/lib/Transforms/Vectorize/VPlan.h
1404

basicBlocks()?

1418

loopBasicBlocks()?

fhahn marked an inline comment as done.Dec 2 2019, 6:09 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
1404

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.

fhahn added a comment.Dec 2 2019, 11:35 AM

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.

gilr added a comment.Dec 7 2019, 1:41 AM

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.

llvm/lib/Transforms/Vectorize/VPlan.h
1404
I think for most iterator ranges we use lower case naming, with underscores

ah, then please keep the current names.