[llvm-dev] RFC: [SmallVector] Adding SVec<T> and Vec<T> convenience wrappers.

Wed Dec 2 15:52:18 PST 2020

So there's lots of fragments to this thread and a lot of emails & I think
it might be simpler if I put thoughts in one place rather than replying to
each subthread.

To Chris's comment(s)

> 1) In a design like today where we have two names for “inlined vector” and
> “not inlined vector”, then I think it is important for “InlinedVector<T>”
> to have at least 1 element.  Defaulting to out of line when using the
> inline vector name betrays intention: it would be better to generate a
> compile time error or warning if the element size is too long, because the
> compiler engineer should use the out of line name.
>
> 2) In a design where we have "one name", then it makes more sense to have
> the default argument type be the “do what I mean” size, which defaults to
> zero if T is too large.
>
> I’m pretty skeptical of #2 as an approach, because inline and out of line
> design points are pretty different when sizeof(T) is 4 or 8, and when
> sizeof(thevector) matters, e.g. in 2D cases.
>

To me, this is more like std::string, which is (2) - and more, to me, about
the contract of the type - std::string doesn't guarantee iterator validity
over swap/move, where std::vector does. If std::vector didn't have this
guarantee, it'd be possible to have small vector optimizations in
std::vector the same way std::string does - without the user-facing
customization, mind you.

So I think (2) has some legs - but the problem for me is that if we name
the type llvm::Vector, which already gets a bit close to std::vector
(unqualified "Vector" seems like fairly subtle code to me - but I'm leaning
towards being OK with that aspect if other peopel are) and it has different
iterator invalidation/move semantics than std::vector, I think that could
be problematic. I don't know what to call it, though - I agree to some
extent with (1) that calling such a thing SmallVector is a bit confusing -
but I don't think it's that confusing, to my mind at least - it doesn't
change the contract, and if the user isn't specifying the small buffer
size, I think we could reasonably say "it's whatever size is good, and that
might be zero" - and importantly if you change the size of the structs
inside the SmallVector, it can change the number of elements (including
down to, or up from, zero) - compared to having it break when you change
the size of a struct, or have it have a weird single element that isn't
what the user probably wanted, I think that (the ability to implicitly go
to zero) would be a good thing.

To J. Y. Knight's comments:

> Why does this need a long tail? We have fancy ast refactoring tooling, and
> a single repository with all the code visible, after all. We can use thar
> to discover all of the missing noexcepts, and add them, all at once. And
> then use a clang tidy to help it remain true.
>

At least to the best of my knowledge, I don't think we have a really nice
clang-tidy integration to help check these things on an ongoing basis -
catching them only in code review (seems we have some kind of clang-tidy
integration in Phabricator) I think is inadequate. If there's some way to
tie in clang-tidy to the build system so it could be enabled at the same
level as clang warnings, I'd be up for that general idea, for sure! (I
think there are maybe still some issues as Mehdi mentioned, with using that
as the route forward for this particular issue, though - in terms of
annotating all the necessary ctors with noexcept... but I'm not set against
it, if it gets more consideration, I'd be up for giving it a closer look -
I like the idea of writing conformantly well performing code using the
standard library, for sure)

On Wed, Dec 2, 2020 at 12:39 PM Duncan P. N. Exon Smith via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

>
>
> On 2020 Dec  1, at 21:04, Chris Lattner <clattner at nondot.org> wrote:
>
> 1) are you, or anyone else, interested in driving an llvm::Vector proposal
> + coding standard change to get us going in the right direction?  I don’t
> think we need a mass migration of the code base, just get the policy
> aligned right plus the new type name to exist.
>
>
> I'll try to get a minimal patch together with docs and send an RFC later
> this week.
>
> (
> I think the initial patch could be as simple as:
> ```
> template <class T> using Vector = SmallVector<T, 0>;
> ```
> but maybe we'd want to rename `SmallVectorImpl` to `VectorImpl`, or maybe
> there are other ideas, but off topic for this thread...
> )
>

Yeah, I'm of mixed feelings here - as mentioned above, naming is hard.
SmallVector<.., 0> certainly has the iterator validity semantics to match
std::vector - but I almost feel like /that/ semantic feature should be the
exception here, and the more common name should cover the "use an inline
buffer if it's useful" situation (including the possibility of that inline
buffer being zero). But for the problem with then having a name really
similar to std::vector but with differing semantics.

Maybe that's what it comes down to - the naming collission/semantic
difference with std::vector is problematic, so SmallVector<0> is the LLVM
std::vector (small sizeof footprint and iterator validity on move).
Renaming SmallVector to somehow let it be more general if the "Small" is
too confusing if the small buffer size could be implicitly zero (which I
think would be valuable, even if we had llvm::Vector where 0 is guaranteed
(small sizeof/+iterator validity on move) - because some/most uses don't
need that guarantee but would still be better off not hardcoding "at least
one" or "zero" size because fo the size of their elements today and would
be better off with a floating guarantee that can pick implicitly based on
the size of the struct on any given day/machine/etc).

- Dave

>
> On 2020 Dec  2, at 09:51, James Y Knight <jyknight at google.com> wrote:
>
> I strongly disagree here. Not wanting to bother to add 'noexcept' to
> user-defined move-constructors is a poor justification for switching to a
> different vector type. Perhaps there are *other* reasons which might
> justify avoiding std::vector, but not that...
>
>
> I'll be sure to mention this alternative in the RFC for llvm::Vector; we
> can talk about it there; maybe you'll convince the rest of us to add the
> `noexcept`s everywhere and then the justification for llvm::Vector would
> indeed be weaker (not completely absent though)...
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201202/1c084a63/attachment.html>