rsandifo-arm added inline comments.
================ Comment at: clang/docs/LanguageExtensions.rst:473 +architectures. The size parameter of a boolean vector type is the number of +bits in the vector (for all non-bool vectors, the number refers to the number +of bytes in the vector). ---------------- simoll wrote: > rsandifo-arm wrote: > > simoll wrote: > > > rsandifo-arm wrote: > > > > simoll wrote: > > > > > lenary wrote: > > > > > > It would be nice if this aside about non-bool vectors was more > > > > > > prominently displayed - it's something I hadn't realised before. > > > > > Yep. that caught me by surprise too. I'll move that sentence to the > > > > > paragraph about GCC vectors above. > > > > Sorry for the extremely late comment. Like @lenary I hadn't thought > > > > about this. I'd assumed that the vector woiuld still be a multiple of > > > > 8 bits in size, but I agree that's probably too restrictive to be the > > > > only option available. > > > > > > > > In that case, would it make sense to add a separate attribute instead? > > > > I think it's too surprising to change the units of the existing > > > > attribute based on the element type. Perhaps we should even make it > > > > take two parameters: the total number of elements, and the number of > > > > bits per element. That might be more natural for some AVX and SVE > > > > combinations. We wouldn't need to supporrt all combinations from the > > > > outset, it's just a question whether we should make the syntax general > > > > enough to support it in future. > > > > > > > > Perhaps we could do both: support `vector_size` for `bool` using byte > > > > sizes (and not allowing subbyte vector lengths), and add a new, more > > > > general attribute that allows subbyte lengths and explicit subbyte > > > > element sizes. > > > > In that case, would it make sense to add a separate attribute instead? > > > > I think it's too surprising to change the units of the existing > > > > attribute based on the element type. Perhaps we should even make it > > > > take two parameters: the total number of elements, and the number of > > > > bits per element. That might be more natural for some AVX and SVE > > > > combinations. We wouldn't need to supporrt all combinations from the > > > > outset, it's just a question whether we should make the syntax general > > > > enough to support it in future. > > > > > > I guess adding a new attribute makes sense mid to long term. For now, i'd > > > want something that just does the job... ie, what is proposed here. We > > > should absolutely document the semantics of vector_size properly.. it > > > already is counterintuitive (broken, if you ask me). > > > > > > > > > > Perhaps we could do both: support vector_size for bool using byte sizes > > > > (and not allowing subbyte vector lengths), and add a new, more general > > > > attribute that allows subbyte lengths and explicit subbyte element > > > > sizes. > > > > > > Disallowing subbyte bool vectors actually makes this more complicated > > > because these types are produced implicitly by compares of (legal) vector > > > types. Consider this: > > > > > > typedef int int3 __attribute__((vector_size(3 * sizeof(int)))); > > > int3 A = ...; > > > int3 B = ...; > > > auto Z = A < B; // vector compare yielding a `bool > > > vector_size(3)`-typed value. > > > > > > > > > Regarding your proposal for a separate subbyte element size and subbyte > > > length, that may or may not make sense but it is surely something that > > > should be discussed more broadly with its own proposal. > > > > Perhaps we could do both: support vector_size for bool using byte sizes > > > > (and not allowing subbyte vector lengths), and add a new, more general > > > > attribute that allows subbyte lengths and explicit subbyte element > > > > sizes. > > > > > > Disallowing subbyte bool vectors actually makes this more complicated > > > because these types are produced implicitly by compares of (legal) vector > > > types. Consider this: > > > > > > typedef int int3 __attribute__((vector_size(3 * sizeof(int)))); > > > int3 A = ...; > > > int3 B = ...; > > > auto Z = A < B; // vector compare yielding a `bool > > > vector_size(3)`-typed value. > > > > Yeah, I understand the need for some way of creating subbyte vectors. I'm > > just not sure using the existing `vector_size` attribute would be the best > > choice for that case. I.e. the objection wasn't based on “keeping things > > simple” but more “keeping things consistent“. > > > > That doesn't mean that the above code should be invalid. It just means > > that it wouldn't be possible to write the type of Z using the existing > > `vector_size` attribute. > > > > (FWIW, `vector_size` was originally a GNUism and GCC stil requires vector > > sizes to be a power of 2, but I realise that isn't relevant here. And the > > same principle applies with s/3/4 in the above example anyway.) > > > > > Regarding your proposal for a separate subbyte element size and subbyte > > > length, that may or may not make sense but it is surely something that > > > should be discussed more broadly with its own proposal. > > > > Yeah, I agree any new attribute would need to be discussed more widely. > > > > Perhaps we could do both: support vector_size for bool using byte sizes > > > > (and not allowing subbyte vector lengths), and add a new, more general > > > > attribute that allows subbyte lengths and explicit subbyte element > > > > sizes. > > > > > > Disallowing subbyte bool vectors actually makes this more complicated > > > because these types are produced implicitly by compares of (legal) vector > > > types. Consider this: > > > > > > typedef int int3 __attribute__((vector_size(3 * sizeof(int)))); > > > int3 A = ...; > > > int3 B = ...; > > > auto Z = A < B; // vector compare yielding a `bool > > > vector_size(3)`-typed value. > > > > Yeah, I understand the need for some way of creating subbyte vectors. I'm > > just not sure using the existing `vector_size` attribute would be the best > > choice for that case. I.e. the objection wasn't based on “keeping things > > simple” but more “keeping things consistent“. > > > > That doesn't mean that the above code should be invalid. It just means > > that it wouldn't be possible to write the type of Z using the existing > > `vector_size` attribute. > > IMHO being able to spell out every type ranks higher than being consistent > with regards to a non-standard extension. That is, you could not do the > assignment of `A < B` in C because there is no way to specify the type > without `auto` or other C++ machinery. > > > > > (FWIW, `vector_size` was originally a GNUism and GCC stil requires vector > > sizes to be a power of 2, but I realise that isn't relevant here. And the > > same principle applies with s/3/4 in the above example anyway.) > > Right, i overlooked the power-of-two constraint. > > How much of a blocker are the subbyte sizes and the power-of-two constraint > to you? I am asking because vector_size with those constraints would be good > enough for us. Keeping the patch as it is mostly makes this extension > potentially more useful to other SIMD/Vector users (and of course saves the > work of changing it). > We could still lift that constraint (or switch to a new attribute) should the > need arise. > How much of a blocker are the subbyte sizes and the power-of-two constraint > to you? I am asking because vector_size with those constraints would be good > enough for us. Keeping the patch as it is mostly makes this extension > potentially more useful to other SIMD/Vector users (and of course saves the > work of changing it). > We could still lift that constraint (or switch to a new attribute) should the > need arise. The non-power-of-2 thing seems fine. It's simply removing a constraint and giving non-power-of-2 sizes their obvious meaning. But I think changing the units in the `vector_size` is too surprising. I think a good indication is that we'd never have designed it like that if we were adding `vector_size` now. I think it's something that would often trip users up and that we'd have to keep explaining away. It's just a personal opinion though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81083/new/ https://reviews.llvm.org/D81083 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
