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: > > > > 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. 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
