aaron.ballman added inline comments.
================
Comment at: clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp:12
+
+template<typename T> struct S { T var; };
+
----------------
craig.topper wrote:
> erichkeane wrote:
> > craig.topper wrote:
> > > aaron.ballman wrote:
> > > > craig.topper wrote:
> > > > > aaron.ballman wrote:
> > > > > > erichkeane wrote:
> > > > > > > craig.topper wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > craig.topper wrote:
> > > > > > > > > > @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.
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > clang/test$ ack template `ack arm_sve_vector -l`
> > > > > > > > > > CodeGenCXX/aarch64-mangle-sve-fixed-vectors.cpp
> > > > > > > > > > 37:template <typename T> struct S {};
> > > > > > > > > >
> > > > > > > > > > SemaCXX/attr-arm-sve-vector-bits.cpp
> > > > > > > > > > 16:template<typename T> struct S { T var; };
> > > > > > > > > > ```
> > > > > > > > > >
> > > > > > > > > > Here is the result for this patch
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > clang/test$ ack template `ack riscv_rvv_vector -l`
> > > > > > > > > > CodeGenCXX/riscv-mangle-rvv-fixed-vectors.cpp
> > > > > > > > > > 48:template <typename T> struct S {};
> > > > > > > > > >
> > > > > > > > > > SemaCXX/attr-riscv-rvv-vector-bits.cpp
> > > > > > > > > > 12:template<typename T> struct S { T var; };
> > > > > > > > > > ```
> > > > > > > > > Thats 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){}
> > > > > > > > >
> > > > > > > > > ```
> > > > > > > > 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.
> > > > > > > Thats 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.
> > > > > > >
> > > > > > > >>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.
> > > > > > > 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.
> > > > > > > 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.
> > > > > >
> > > > > > 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.
> > > > > This is what's in ARM's ACLE documentation:
> > > > >
> > > > >
> > > > >
> > > > > > The ACLE only defines the effect of the attribute if all of the
> > > > > > following are true:
> > > > > > 1. the attribute is attached to a single SVE vector type (such as
> > > > > > svint32_t) or to the SVE predicate
> > > > > > type svbool_t;
> > > > > > 2. the arguments “…” consist of a single nonzero integer constant
> > > > > > expression (referred to as N below); and
> > > > > > 3. N==__ARM_FEATURE_SVE_BITS.
> > > > > > In other cases the implementation must do one of the following:
> > > > > > • ignore the attribute; a warning would then be appropriate, but is
> > > > > > not required
> > > > > > • reject the program with a diagnostic
> > > > > > • extend requirement (3) above to support other values of N besides
> > > > > > __ARM_FEATURE_SVE_BITS
> > > > > > • process the attribute in accordance with a later revision of the
> > > > > > ACLE
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > So there's a bullet in there that allows an implementation to support
> > > > > other values, but it is not required.
> > > > Thank 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
> > > > ```
> > > >
> > > It looks like it doesn't work for that case.
> > THAT is super unfortunate, and really should work in this case. The SVE
> > implementers could probably help out here.
> Is that blocking for this patch?
It'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).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145088/new/
https://reviews.llvm.org/D145088
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
