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: > > > > > 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. > > 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. > > Ok. To make sure we are talking about the same thing here, you are suggesting: > > typedef bool bool16 __attribute__((vector_size(2))); > > Would be a vector of 16 bits stored in two bytes. Correct? If so, that's fine > with me and i'll change the patch right away. > > 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. > > Ok. To make sure we are talking about the same thing here, you are suggesting: > > typedef bool bool16 __attribute__((vector_size(2))); > > Would be a vector of 16 bits stored in two bytes. Correct? Yeah. And if anyone wants something like a vector of 4 bools in a 4-bit vector (which would certainly be reasonable in principle), we could leave that to some future, more general attribute syntax that doesn't have the same historical baggage as `vector_size`. 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
