simoll 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). ---------------- 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. 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
