Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline
Changeset View
Standalone View
clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp
- This file was added.
// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64x -ffreestanding -fsyntax-only -verify -std=c++11 -mvscale-min=4 -mvscale-max=4 -Wconversion %s | |||||
// expected-no-diagnostics | |||||
#include <stdint.h> | |||||
typedef __rvv_int8m1_t vint8m1_t; | |||||
typedef vint8m1_t fixed_int8m1_t __attribute__((riscv_rvv_vector_bits(__riscv_v_fixed_vlen))); | |||||
typedef int8_t gnu_int8m1_t __attribute__((vector_size(__riscv_v_fixed_vlen / 8))); | |||||
template<typename T> struct S { T var; }; | |||||
S<fixed_int8m1_t> s; | |||||
craig.topper: @erichkeane does this cover the dependent case or were you looking for something else?
Here… | |||||
Not Done ReplyInline ActionsThats unfortunate, and I wish I'd thought of it at the time/been more active reviewing the SVE stuff then. Really what I'm looking for is: template<int N> struct Whatever { using Something = char __attribute((riscv_rvv_vector_bits(N))); }; void Func(Whatever<5>::Something MyVar){} erichkeane: Thats unfortunate, and I wish I'd thought of it at the time/been more active reviewing the SVE… | |||||
That does not appear to work. $ ./bin/clang test.cpp --target=riscv64 -march=rv64gcv -mrvv-vector-bits=zvl test.cpp:3:41: error: 'riscv_rvv_vector_bits' attribute requires an integer constant using Something = char __attribute((riscv_rvv_vector_bits(N))); It's not very useful as a template parameter. There's only one value that works and that's whatever __RISCV_RVV_VLEN_BITS is set to. craig.topper: That does not appear to work.
```
$ ./bin/clang test.cpp --target=riscv64 -march=rv64gcv -mrvv… | |||||
Not Done ReplyInline ActionsThats really unfortunate, but it makes me wonder what DependentVectorType is for in this case, or the handling of said things. Because I would expect: template<typename T, int Size> using RiscvVector = T __attribute__((risv_rvv_vector_bits(Size))); RiscvVector<char, <TheRightAnswer>> Foo; to be useful. Even if not, I'd expect: template<typename T> using RiscvVector = T __attribute__((risv_rvv_vector_bits(TheRightAnswer))); RiscvVector<char> Foo; to both work.
This makes me wonder why this attribute takes an integer constant anyway, if it is just a 'guess what the right answer is!' sorta thing. Seems to me this never should have taken a parameter. erichkeane: Thats really unfortunate, but it makes me wonder what `DependentVectorType ` is for in this… | |||||
Not Done ReplyInline Actions
Can you help me understand why the argument exists then? We're pretty inconsistent about attribute arguments properly handling things like constant expressions vs integer literals, but the trend lately is to accept a constant expression rather than only a literal because of how often users like to give names to literals and how much more constexpr code we're seeing in the wild. aaron.ballman: > It's not very useful as a template parameter. There's only one value that works and that's… | |||||
This is what's in ARM's ACLE documentation:
So there's a bullet in there that allows an implementation to support other values, but it is not required. craig.topper: This is what's in ARM's ACLE documentation:
> The ACLE only defines the effect of the… | |||||
Not Done ReplyInline ActionsThank you, the current design makes more sense to me now. I'm less concerned about whether we support dependent values for this attribute argument. If we start to support values of N other than __ARM_FEATURE_SVE_BITS then it might make sense to care about it at that point. But I don't think users are going to do stuff like: template <int N> using fixed_int8m1_t __attribute__((riscv_rvv_vector_bits(N))) = vint8m1_t; fixed_int8m1_t<__ARM_FEATURE_SVE_BITS> foo; However, it is still important to test that the type attribute works in a situation like: template <typename Ty> using Something = Ty __attribute__((riscv_rvv_vector_bits(__ARM_FEATURE_SVE_BITS))); // Ensure that Something is correctly attributed, that the underlying type for Ty is valid for the attribute, etc aaron.ballman: Thank you, the current design makes more sense to me now. I'm less concerned about whether we… | |||||
It looks like it doesn't work for that case. craig.topper: It looks like it doesn't work for that case. | |||||
Not Done ReplyInline ActionsTHAT is super unfortunate, and really should work in this case. The SVE implementers could probably help out here. erichkeane: THAT is super unfortunate, and really should work in this case. The SVE implementers could… | |||||
Is that blocking for this patch? craig.topper: Is that blocking for this patch? | |||||
Not Done ReplyInline ActionsIt's @erichkeane 's call, but personally, I don't think that should block this patch (only because it's a second instance of an existing issue and this patch is quite large already, basically), but it definitely needs to be solved here and for SVE rather than kicking the can down the road to someone else. New types need to fit into the type system cleanly and that includes being able to use them from templates. So how about this for a compromise: file an issue (or more than one if you'd prefer) to fix these attributed types up so we don't forget to do it, and plan to work on that issue ASAP (or rope someone else into it). aaron.ballman: It's @erichkeane 's call, but personally, I don't think that should block this patch (only… | |||||
Not Done ReplyInline Actions
I think this is an acceptable compromise to me. erichkeane: >>So how about this for a compromise: file an issue (or more than one if you'd prefer) to fix… | |||||
// Test implicit casts between VLA and VLS vectors | |||||
vint8m1_t to_vint8m1_t(fixed_int8m1_t x) { return x; } | |||||
fixed_int8m1_t from_vint8m1_t(vint8m1_t x) { return x; } | |||||
// Test implicit casts between GNU and VLA vectors | |||||
vint8m1_t to_vint8m1_t__from_gnu_int8m1_t(gnu_int8m1_t x) { return x; } | |||||
gnu_int8m1_t from_vint8m1_t__to_gnu_int8m1_t(vint8m1_t x) { return x; } | |||||
// Test implicit casts between GNU and VLS vectors | |||||
fixed_int8m1_t to_fixed_int8m1_t__from_gnu_int8m1_t(gnu_int8m1_t x) { return x; } | |||||
gnu_int8m1_t from_fixed_int8m1_t__to_gnu_int8m1_t(fixed_int8m1_t x) { return x; } |
@erichkeane does this cover the dependent case or were you looking for something else?
Here are on the only mentions of template I see in SVE tests that use this attribute.
Here is the result for this patch