This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Add SmallVector constructor to allow creation of SmallVector<T> from ArrayRef of items convertible to type T
ClosedPublic

Authored by yurai007 on Jul 21 2022, 6:50 AM.

Diff Detail

Event Timeline

yurai007 created this revision.Jul 21 2022, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 6:50 AM
yurai007 requested review of this revision.Jul 21 2022, 6:50 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 21 2022, 6:50 AM
yurai007 added inline comments.Jul 21 2022, 6:53 AM
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)

That's because of "error: conditional expression is ambiguous; 'llvm::SmallVector<llvm::Constant *, 32>' can be converted to 'ArrayRef<llvm::Constant *>' and vice versa". Need to check if there is easier workaround.

Instead adding one more constructor another option is tweaking existing SmallVector(const iterator_range<RangeTy> &R) one in following way:

-  template <typename RangeTy>
-  explicit SmallVector(const iterator_range<RangeTy> &R)
-      : SmallVectorImpl<T>(N) {
+  template <typename Range, typename = std::enable_if_t<std::is_convertible<
+                                ValueTypeFromRangeType<Range>, T>::value>>
+  SmallVector(const Range &R) : SmallVectorImpl<T>(N) {
     this->append(R.begin(), R.end());
}

That would unlock possibility of creating SmallVector not only from ArrayRef but from every range as long as types are convertible.
I didn't explore this way further, I'm not sure but I believe it should work. It's more than just "adding ArrayRef constructor" but if you are interested I can check where it leads.

nikic added inline comments.Jul 21 2022, 7:16 AM
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)

Would making the ctor explicit help?

yurai007 added inline comments.Jul 21 2022, 7:29 AM
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)

Nope :( Making constructor explicit disables implicit conversion so we cannot do things like: SmallVector<int, 16> NewMask = Mask; anymore.

barannikov88 added inline comments.
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)

And leaving it implicit hides the fact of possible memory allocation, which is not cheap. I think absense of such constructor is by-design.

Making it explicit is making it redundant, because there is already a constructor which accepts begin / end, and one that accepts range (note that it is explicit!). It won't save on typing, either.

I'm not in favor of this patch, but my word does not count much, so I won't block it. I'd suggest you, however, to request review of core maintainers.

nikic added inline comments.Jul 21 2022, 8:27 AM
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)

Nope :( Making constructor explicit disables implicit conversion so we cannot do things like: SmallVector<int, 16> NewMask = Mask; anymore.

I think that's fine. Similar to the existing iterator_range constructor, we would require SmallVector<int, 16> NewMask(Mask), which seems like the idiomatic way to write it anyway?

Making it explicit is making it redundant, because there is already a constructor which accepts begin / end, and one that accepts range (note that it is explicit!). It won't save on typing, either.

It is not redundant. It ensures that iterator_range and ArrayRef can be freely substituted. Switching iterator_range to ArrayRef currently requires going through a lot of SmallVector constructors and replacing them with less readable code. The alternative to this change is D129988, which looks like a significant regression in code quality.

barannikov88 added inline comments.Jul 21 2022, 8:29 AM
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)

Please also consider the fact that this is API breaking change due to this "conditional expression is amiguous" error. Many external projects depend on LLVM/ADT, and all of them will have to adapt this change.

barannikov88 added inline comments.Jul 21 2022, 8:33 AM
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)

It ensures that iterator_range and ArrayRef can be freely substituted. Switching iterator_range to ArrayRef currently requires going through a lot of SmallVector constructors and replacing them with less readable code.

Ok, makes sense.

yurai007 added inline comments.Jul 21 2022, 9:10 AM
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)
Nope :( Making constructor explicit disables implicit conversion so we cannot do things like: SmallVector<int, 16> NewMask = Mask; anymore.

I think that's fine. Similar to the existing iterator_range constructor, we would require SmallVector<int, 16> NewMask(Mask), which seems like the idiomatic way to write it anyway?

You are right, explicit works when we use brackets instead assignment. I'm gonna add it to constructor and adjust rest of patch.

yurai007 updated this revision to Diff 446773.Jul 22 2022, 3:51 AM
yurai007 edited the summary of this revision. (Show Details)

Update constructor definition with explicit to avoid implicit conversions and remove ambiguity workarounds.
Adjust constructor users and add unit tests.

yurai007 marked an inline comment as done.Jul 22 2022, 3:58 AM
yurai007 added inline comments.
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)

@barannikov88: after addressing @nikic comment there is no API breaking change anymore. As far I can tell SmallVector API is only extended and still backward-compatible. Hope it looks better now.

yurai007 retitled this revision from [WIP] Add SmallVector constructor to allow creation of SmallVector<T> from ArrayRef of items convertible to type T to [NFC] Add SmallVector constructor to allow creation of SmallVector<T> from ArrayRef of items convertible to type T.Jul 22 2022, 4:07 AM

Drop WIP tag.

barannikov88 added inline comments.Jul 22 2022, 8:35 AM
clang/lib/CodeGen/CGExprConstant.cpp
504 ↗(On Diff #446472)

Looks good, thank you. Still, I would suggest requesting a review from more experienced developers than me.

barannikov88 added inline comments.Jul 22 2022, 8:37 AM
llvm/include/llvm/ADT/SmallVector.h
35

Should be typename T. class T is archaic.

I think my only concern with this change is that it will also allow some implicit ArrayRef constructors. For example, this will permit creating a SmallVector from std::array, std::vector, or just T -- but only if ArrayRef is in scope. This seems somewhat dangerous.

I'm not sure if C++ provides any good way to avoid that, short of explicitly marking certain constructors as deleted?

I'm wondering if it might not be better to make this a fully generic overload that accepts any range (i.e., anything with begin() and end()). Paradoxically, this will likely be less permissive in practice than having explicit iterator_range and ArrayRef overloads, because it allows less implicit conversions.

I think my only concern with this change is that it will also allow some implicit ArrayRef constructors. For example, this will permit creating a SmallVector from std::array, std::vector, or just T -- but only if ArrayRef is in scope. This seems somewhat dangerous.

Maybe I'm missing something, but is it right? It appears that only one implicit conversion in constructor is allowed: https://stackoverflow.com/questions/63320366/double-implicit-conversion-in-c
I think going from vector/array to SmallVector is ill-formed unless we explicitly create temporary ArrayRef in-between: https://godbolt.org/z/Geqbajf1x

nikic accepted this revision.Jul 29 2022, 2:06 AM

LGTM

I think my only concern with this change is that it will also allow some implicit ArrayRef constructors. For example, this will permit creating a SmallVector from std::array, std::vector, or just T -- but only if ArrayRef is in scope. This seems somewhat dangerous.

Maybe I'm missing something, but is it right? It appears that only one implicit conversion in constructor is allowed: https://stackoverflow.com/questions/63320366/double-implicit-conversion-in-c
I think going from vector/array to SmallVector is ill-formed unless we explicitly create temporary ArrayRef in-between: https://godbolt.org/z/Geqbajf1x

You're right... Everything's good then.

This revision is now accepted and ready to land.Jul 29 2022, 2:06 AM
yurai007 updated this revision to Diff 449963.Aug 4 2022, 7:05 AM
yurai007 edited the summary of this revision. (Show Details)

Rebase and fix last comment. Remove dependency to https://reviews.llvm.org/D129781 by moving relevant part of change here.

nikic accepted this revision.Aug 4 2022, 7:12 AM

Still LGTM

yurai007 marked an inline comment as done.Aug 4 2022, 7:13 AM
fhahn added a subscriber: fhahn.Aug 4 2022, 7:19 AM

Looks like a nice improvement, thanks!

llvm/unittests/ADT/SmallVectorTest.cpp
869–870

This seems like an unrelated change unless I am missing something? If so, this could likely be pulled out and just submitted separately as NFC

yurai007 marked an inline comment as done.
yurai007 added inline comments.
llvm/unittests/ADT/SmallVectorTest.cpp
869–870
yurai007 updated this revision to Diff 450069.Aug 4 2022, 11:01 AM
yurai007 marked an inline comment as done.

Rebase to last SmallVector NFC.

yurai007 updated this revision to Diff 450259.Aug 5 2022, 1:54 AM

Fix std::initializer_list lifetime issue in test case revealed on CI

This revision was landed with ongoing or failed builds.Aug 5 2022, 4:38 AM
This revision was automatically updated to reflect the committed changes.