erichkeane added a comment.
This patch LGTM given the above compromise, but one of the clang-codegen needs
to take a look to accept.
================
Comment at: clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp:12
+
+template<typename T> struct S { T var; };
+
----------------
aaron.ballman wrote:
> 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).
>>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).
I think this is an acceptable compromise to me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145088/new/
https://reviews.llvm.org/D145088
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
