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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145088