Guard local variable declaration for RVV intrinsic types.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This checking already happens when the declaration is actually *used*, so I question whether we need to do the check at all (declared but unused variables seem like an edge case to me): https://godbolt.org/z/e4Y8qKMrW
What is the behavior of the example I linked with your patch? Do we now issue the diagnostic twice (once on the declaration and again on the use)?
Yes. The missing behavior here is the check when variables are declared and not used. This patch lets the compiler emit error upon declaration.
$ cat test.c #include <riscv_vector.h> void bar(void) { vint64m1_t i64m1; (void)i64m1; } eopc@sw02:/scratch/eopc/upstream-llvm-project2/build$ bin/clang -march=rv64g_zve32x test.c test.c:4:14: error: RISC-V type 'vint64m1_t' (aka '__rvv_int64m1_t') requires the 'zve64x' extension 4 | vint64m1_t i64m1; | ^ test.c:5:9: error: RISC-V type 'vint64m1_t' (aka '__rvv_int64m1_t') requires the 'zve64x' extension 5 | (void)i64m1; | ^ 2 errors generated.
@aaron.ballman The backend crashes at -O0 when scalable vector variables are declared because the stack frame code doesn't expect to see them when the vector extension isn't enabled.
clang/lib/Sema/SemaDecl.cpp | ||
---|---|---|
8781 | This feels heavy handed for not RISC-V vector types. Can we put the RISC-V code from checkTypeSupport in a function and call it? |
Extract RVV type check as a separate function as Craig has commented.
Add test case from Aaron's comment.
clang/lib/Sema/SemaDecl.cpp | ||
---|---|---|
8780 | Add a blank line before this to separate it from SVE |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4968 | Do we need to diagnose the use of any RVV type without at least zve32x? |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4968 | If users will be using RVV intrinsic types, they will need to include <riscv_vector.h>, and the header is guarded by zve32x. So I think we don't need the check for at least zve32x. $ bin/clang -march=rv64g -emit-llvm -S -o - test.c In file included from test.c:1: /scratch/eopc/upstream-llvm-project2/build/lib/clang/17/include/riscv_vector.h:18:2: error: "Vector intrinsics require the vector extension." 18 | #error "Vector intrinsics require the vector extension." | ^ 1 error generated. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4968 | The __rvv prefixed names are still available and do crash the backend if you use them without zve32x. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4967 | What about the bool types? Is it sufficient to check Zve32x is enabled after all the other checks? The caller already checked for it being an RVV type so if we are in the function the type is RVV. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4967 | I don't think this comment was addressed |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4967 | Sorry I misread and only added checks for vint{8/16/32}*_t. Added checks to bool and coverage under riscv-vector-zve32x-check.c. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4969 | We don't to check exactly what type it is. We can do if (!TI.hasFeature("zve32x")) after all the other checks. The caller already checked it was an RVV type. We don't need to know anymore about the type, they all require zve32x. |
Simplify check with the presumption that caller has called isRVVType() before calling isRVVTypeSupport.
That's a good reason to diagnose the declaration, but this makes the usage diagnostic very chatty -- if someone forgets to enable the extension, they'll get a diagnostic for each time they use the type and each time they declare something with the type, and there are likely to be more uses than there are declarations. No need to drown the user in repeated messages, so I think the diagnostic on usage should be disabled. WDYT?
What about the bool types? Is it sufficient to check Zve32x is enabled after all the other checks? The caller already checked for it being an RVV type so if we are in the function the type is RVV.