[Clang][RISCV] Check type support for local variable declaration of RVV type

[eopXD]

Summary

Guard local variable declaration for RVV intrinsic types.

[aaron.ballman]

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)?

[eopXD]

In D153510#4440784, @aaron.ballman wrote:

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.

[craig.topper]

@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.

[craig.topper]

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?

[eopXD]

Extract RVV type check as a separate function as Craig has commented.
Add test case from Aaron's comment.

[eopXD]

Rebase to latest main.

[craig.topper]

clang/lib/Sema/SemaDecl.cpp

8780

Add a blank line before this to separate it from SVE

[eopXD]

Add a blank line to separate from SVE.

[craig.topper]

clang/lib/Sema/SemaChecking.cpp

4968

Do we need to diagnose the use of any RVV type without at least zve32x?

[eopXD]

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.

[craig.topper]

clang/lib/Sema/SemaChecking.cpp

4968

The __rvv prefixed names are still available and do crash the backend if you use them without zve32x.

[eopXD]

Add check for RVV types that require at least zve32x

[craig.topper]

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.

[craig.topper]

Does the code do the right FP checks for tuples?

[eopXD]

Add test coverage for tuple types.

[craig.topper]

clang/lib/Sema/SemaChecking.cpp

4967

I don't think this comment was addressed

[eopXD]

Add check for RVV boolean types and simplify if-condition under checkRVVTypeSupport.

[eopXD]

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.

[craig.topper]

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.

[eopXD]

Simplify check with the presumption that caller has called isRVVType() before calling isRVVTypeSupport.

[craig.topper]

LGTM

[aaron.ballman]

In D153510#4441541, @craig.topper wrote:

@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.

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?

clang/include/clang/Sema/Sema.h

private:
   bool CheckX86BuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
                                    CallExpr *TheCall);
   bool CheckPPCBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
                                    CallExpr *TheCall);
   bool CheckAMDGCNBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall);
   bool CheckRISCVLMUL(CallExpr *TheCall, unsigned ArgNum);
   bool CheckRISCVBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
                                      CallExpr *TheCall);
+  void checkRVVTypeSupport(QualType Ty, SourceLocation Loc, ValueDecl *D);
   bool CheckLoongArchBuiltinFunctionCall(const TargetInfo &TI,
                                          unsigned BuiltinID, CallExpr *TheCall);
   bool CheckWebAssemblyBuiltinFunctionCall(const TargetInfo &TI,
                                            unsigned BuiltinID,
                                            CallExpr *TheCall);
   bool CheckNVPTXBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
                                      CallExpr *TheCall);
 

clang/lib/Sema/Sema.cpp

if (IsRetTy && !TI.hasFPReturn() && (IsDouble || IsFloat)) {
           << TI.getTriple().str()) {
         if (D)
           D->setInvalidDecl();
       }
       if (D)
         targetDiag(D->getLocation(), diag::note_defined_here, FD) << D;
     }
 
-    // RISC-V vector builtin types (RISCVVTypes.def)
-    if (Ty->isRVVType(/* Bitwidth */ 64, /* IsFloat */ false) &&
-        !TI.hasFeature("zve64x"))
-      Diag(Loc, diag::err_riscv_type_requires_extension, FD) << Ty << "zve64x";
-    if (Ty->isRVVType(/* Bitwidth */ 16, /* IsFloat */ true) &&
-        !TI.hasFeature("experimental-zvfh"))
-      Diag(Loc, diag::err_riscv_type_requires_extension, FD)
-          << Ty << "zvfh";
-    if (Ty->isRVVType(/* Bitwidth */ 32, /* IsFloat */ true) &&
-        !TI.hasFeature("zve32f"))
-      Diag(Loc, diag::err_riscv_type_requires_extension, FD) << Ty << "zve32f";
-    if (Ty->isRVVType(/* Bitwidth */ 64, /* IsFloat */ true) &&
-        !TI.hasFeature("zve64d"))
-      Diag(Loc, diag::err_riscv_type_requires_extension, FD) << Ty << "zve64d";
+    if (Ty->isRVVType())
+      checkRVVTypeSupport(Ty, Loc, D);
 
     // Don't allow SVE types in functions without a SVE target.
     if (Ty->isSVESizelessBuiltinType() && FD && FD->hasBody()) {
       llvm::StringMap<bool> CallerFeatureMap;
       Context.getFunctionFeatureMap(CallerFeatureMap, FD);
       if (!Builtin::evaluateRequiredTargetFeatures(
           "sve", CallerFeatureMap))
         Diag(D->getLocation(), diag::err_sve_vector_in_non_sve_target) << Ty;

clang/lib/Sema/SemaChecking.cpp

case WebAssembly::BI__builtin_wasm_table_fill:
     return BuiltinWasmTableFill(TheCall);
   case WebAssembly::BI__builtin_wasm_table_copy:
     return BuiltinWasmTableCopy(TheCall);
   }
 
   return false;
 }
 
+void Sema::checkRVVTypeSupport(QualType Ty, SourceLocation Loc, ValueDecl *D) {
+  const TargetInfo &TI = Context.getTargetInfo();
+  if (Ty->isRVVType(/* Bitwidth */ 64, /* IsFloat */ false) &&
+      !TI.hasFeature("zve64x"))
+    Diag(Loc, diag::err_riscv_type_requires_extension, D) << Ty << "zve64x";
+  if (Ty->isRVVType(/* Bitwidth */ 16, /* IsFloat */ true) &&
+      !TI.hasFeature("experimental-zvfh"))
+    Diag(Loc, diag::err_riscv_type_requires_extension, D) << Ty << "zvfh";
+  if (Ty->isRVVType(/* Bitwidth */ 32, /* IsFloat */ true) &&
+      !TI.hasFeature("zve32f"))
+    Diag(Loc, diag::err_riscv_type_requires_extension, D) << Ty << "zve32f";
+  if (Ty->isRVVType(/* Bitwidth */ 64, /* IsFloat */ true) &&
+      !TI.hasFeature("zve64d"))
+    Diag(Loc, diag::err_riscv_type_requires_extension, D) << Ty << "zve64d";
+}
+
 bool Sema::CheckNVPTXBuiltinFunctionCall(const TargetInfo &TI,
                                          unsigned BuiltinID,
                                          CallExpr *TheCall) {
   switch (BuiltinID) {
   case NVPTX::BI__nvvm_cp_async_ca_shared_global_4:
   case NVPTX::BI__nvvm_cp_async_ca_shared_global_8:
   case NVPTX::BI__nvvm_cp_async_ca_shared_global_16:
   case NVPTX::BI__nvvm_cp_async_cg_shared_global_16:

clang/lib/Sema/SemaDecl.cpp

if (T->isSVESizelessBuiltinType() && isa<FunctionDecl>(CurContext)) {
     Context.getFunctionFeatureMap(CallerFeatureMap, FD);
     if (!Builtin::evaluateRequiredTargetFeatures(
         "sve", CallerFeatureMap)) {
       Diag(NewVD->getLocation(), diag::err_sve_vector_in_non_sve_target) << T;
       NewVD->setInvalidDecl();
       return;
     }
   }
+
+  if (T->isRVVType())
+    checkRVVTypeSupport(T, NewVD->getLocation(), cast<ValueDecl>(CurContext));
 }
 
 /// Perform semantic checking on a newly-created variable
 /// declaration.
 ///
 /// This routine performs all of the type-checking required for a
 /// variable declaration once it has been built. It is used both to
 /// check variables after they have been parsed and their declarators

clang/test/Sema/riscv-types.c

-// RUN: %clang_cc1 -triple riscv64 -target-feature +v -ast-print %s \
-// RUN:    | FileCheck %s
+// RUN: %clang_cc1 -triple riscv64 -target-feature +v \
+// RUN: -target-feature +experimental-zvfh -ast-print %s | FileCheck %s
 
 void bar(void) {
   // CHECK: __rvv_int64m1_t x0;
   __rvv_int64m1_t x0;
 
   // CHECK: __rvv_float64m1_t x1;
   __rvv_float64m1_t x1;
 

clang/test/Sema/riscv-vector-float16-check.c

 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d \
 // RUN:   -target-feature +v -target-feature +zfh \
 // RUN:   -disable-O0-optnone -o - -fsyntax-only %s -verify 
 // REQUIRES: riscv-registered-target
 #include <riscv_vector.h>
 
 vfloat16m1_t foo() { /* expected-error {{RISC-V type 'vfloat16m1_t' (aka '__rvv_float16m1_t') requires the 'zvfh' extension}} */
 } /* expected-warning {{non-void function does not return a value}}*/
+
+void bar(void) {
+  vfloat16m1_t f16m1; /* expected-error {{RISC-V type 'vfloat16m1_t' (aka '__rvv_float16m1_t') requires the 'zvfh' extension}} */
+
+  (void)f16m1; /* expected-error {{RISC-V type 'vfloat16m1_t' (aka '__rvv_float16m1_t') requires the 'zvfh' extension}} */
+}

clang/test/Sema/riscv-vector-float32-check.c

 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d \
 // RUN:   -target-feature +zve32x -target-feature +zfh \
 // RUN:   -disable-O0-optnone -o - -fsyntax-only %s -verify 
 // REQUIRES: riscv-registered-target
 #include <riscv_vector.h>
 
 vfloat32m1_t foo() { /* expected-error {{RISC-V type 'vfloat32m1_t' (aka '__rvv_float32m1_t') requires the 'zve32f' extension}} */
 } /* expected-warning {{non-void function does not return a value}}*/
+
+void bar(void) {
+  vfloat32m1_t f32m1; /* expected-error {{RISC-V type 'vfloat32m1_t' (aka '__rvv_float32m1_t') requires the 'zve32f' extension}} */
+
+  (void)f32m1; /* expected-error {{RISC-V type 'vfloat32m1_t' (aka '__rvv_float32m1_t') requires the 'zve32f' extension}} */
+}

clang/test/Sema/riscv-vector-float64-check.c

 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d \
 // RUN:   -target-feature +zve64f -target-feature +zfh \
 // RUN:   -disable-O0-optnone -o - -fsyntax-only %s -verify 
 // REQUIRES: riscv-registered-target
 #include <riscv_vector.h>
 
 vfloat64m1_t foo() { /* expected-error {{RISC-V type 'vfloat64m1_t' (aka '__rvv_float64m1_t') requires the 'zve64d' extension}} */
 } /* expected-warning {{non-void function does not return a value}}*/
+
+void bar(void) {
+  vfloat64m1_t f64m1; /* expected-error {{RISC-V type 'vfloat64m1_t' (aka '__rvv_float64m1_t') requires the 'zve64d' extension}} */
+
+  (void)f64m1; /* expected-error {{RISC-V type 'vfloat64m1_t' (aka '__rvv_float64m1_t') requires the 'zve64d' extension}} */
+}

clang/test/Sema/riscv-vector-int64-check.c

 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d \
 // RUN:   -target-feature +zve32x -target-feature +zfh \
 // RUN:   -disable-O0-optnone -o - -fsyntax-only %s -verify 
 // REQUIRES: riscv-registered-target
 #include <riscv_vector.h>
 
 vint64m1_t foo() { /* expected-error {{RISC-V type 'vint64m1_t' (aka '__rvv_int64m1_t') requires the 'zve64x' extension}} */
 } /* expected-warning {{non-void function does not return a value}}*/
+
+void bar(void) {
+  vint64m1_t i64m1; /* expected-error {{RISC-V type 'vint64m1_t' (aka '__rvv_int64m1_t') requires the 'zve64x' extension}} */
+
+  (void)i64m1; /* expected-error {{RISC-V type 'vint64m1_t' (aka '__rvv_int64m1_t') requires the 'zve64x' extension}} */
+}