This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Introduce `map_to_vector` helper
ClosedPublic

Authored by laszlokindrat on Mar 6 2023, 8:18 AM.

Details

Summary

The following pattern is common in the llvm codebase, as well as in downstream projects:

llvm::to_vector(llvm::map_range(container, lambda))

This patch introduces a shortcut for this called map_to_vector.

This template depends on both llvm/ADT/SmallVector.h and llvm/ADT/STLExtras.h, and since these are both relatively large and do not depend on each other, the map_to_vector helper is placed in a new header under llvm/ADT/SmallVectorExtras.h. I am happy to change this if reviewers can suggest a better location.

Only a handful of use cases have been updated to use the new helper, but I'm happy to update across the codebase (either in this patch, or in subsequent diffs) if this patch is accepted.

Diff Detail

Event Timeline

laszlokindrat created this revision.Mar 6 2023, 8:18 AM
laszlokindrat requested review of this revision.Mar 6 2023, 8:18 AM
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
bzcheeseman accepted this revision.EditedMar 6 2023, 9:30 AM

Nice, thank you.

This revision is now accepted and ready to land.Mar 6 2023, 9:30 AM
kuhar added a subscriber: kuhar.May 6 2023, 2:25 PM
kuhar added inline comments.
llvm/include/llvm/ADT/SmallVectorExtras.h
24–25

I'd expect this to optionally accept small size, just like to_vector<N>(...) does. Is there a specific reason why only the default size is allowed?

Also, do you plan on adding map_to_vector_of? IIRC to_vector_of was required for some common mlir functions like DenseElementAttribute::getValues<T>()

laszlokindrat added inline comments.May 9 2023, 6:02 AM
llvm/include/llvm/ADT/SmallVectorExtras.h
24–25

I'd expect this to optionally accept small size, just like to_vector<N>(...) does. Is there a specific reason why only the default size is allowed?

No reason other than I prefer to add complexity incrementally. But I agree that this would be nice, so I will add it in a subsequent patch.

Also, do you plan on adding map_to_vector_of?

I can't really find any use cases for this in the llvm monorepo. Do you have a downstream project that could benefit from it? I usually like to add this kind of sugar if/when there are lots of places that can benefit from it.

kuhar added inline comments.May 10 2023, 7:25 AM
llvm/include/llvm/ADT/SmallVectorExtras.h
24–25

I can't really find any use cases for this in the llvm monorepo. Do you have a downstream project that could benefit from it? I usually like to add this kind of sugar if/when there are lots of places that can benefit from it.

I think I wanted to use to_vector(map(...)) here https://github.com/llvm/llvm-project/blob/890aa28f1c9a502ad012d83ad4d6ad60f75efccb/mlir/lib/Dialect/Vector/IR/VectorOps.cpp#L2671C10-L2687 and https://github.com/llvm/llvm-project/blob/890aa28f1c9a502ad012d83ad4d6ad60f75efccb/mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp#L201-L204 (in reduce or accumulate) but couldn't at the time. I think this can be a bit of a chicken and egg problem -- you may not be able to find many places that would immediately benefit form it because folks have already worked around it.

laszlokindrat added inline comments.May 15 2023, 12:24 PM
llvm/include/llvm/ADT/SmallVectorExtras.h
24–25

See https://reviews.llvm.org/D150601 for a patch that adds the ability to specify the size of the resulting vector.

(my 2c would be that I'm not sure the wrapper pulls its weight - writing the two functions calls out doesn't seem too bad, but no big deal either way)