[PATCH] D145088: [RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/test/SemaCXX/attr-riscv-rvv-vector-bits.cpp:12
+
+template<typename T> struct S { T var; };
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145088/new/

https://reviews.llvm.org/D145088