[clang] Implement resource binding type prefix mismatch errors (PR #87578)

Joshua Batista via cfe-commits cfe-commits at lists.llvm.org
Wed Apr 10 18:12:04 PDT 2024


https://github.com/bob80905 updated https://github.com/llvm/llvm-project/pull/87578

>From 3960050439964fe3c0536696490b284a6c470cd1 Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Wed, 3 Apr 2024 13:15:59 -0700
Subject: [PATCH 1/4] implement binding type error for t/cbuffers and rwbuffers

---
 .../clang/Basic/DiagnosticSemaKinds.td        |  1 +
 clang/lib/Sema/HLSLExternalSemaSource.cpp     | 19 +++--
 clang/lib/Sema/SemaDeclAttr.cpp               | 73 ++++++++++++++++++-
 .../resource_binding_attr_error_mismatch.hlsl | 65 +++++++++++++++++
 4 files changed, 149 insertions(+), 9 deletions(-)
 create mode 100644 clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 51af81bf1f6fc5..9a0946276f80fb 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12149,6 +12149,7 @@ def err_hlsl_missing_semantic_annotation : Error<
 def err_hlsl_init_priority_unsupported : Error<
   "initializer priorities are not supported in HLSL">;
 
+def err_hlsl_mismatching_register_type_and_name: Error<"invalid register name prefix '%0' for register type '%1' (expected '%2')">;
 def err_hlsl_unsupported_register_type : Error<"invalid resource class specifier '%0' used; expected 'b', 's', 't', or 'u'">;
 def err_hlsl_unsupported_register_number : Error<"register number should be an integer">;
 def err_hlsl_expected_space : Error<"invalid space specifier '%0' used; expected 'space' followed by an integer, like space1">;
diff --git a/clang/lib/Sema/HLSLExternalSemaSource.cpp b/clang/lib/Sema/HLSLExternalSemaSource.cpp
index 1a1febf7a35241..479689ec82dcee 100644
--- a/clang/lib/Sema/HLSLExternalSemaSource.cpp
+++ b/clang/lib/Sema/HLSLExternalSemaSource.cpp
@@ -119,8 +119,10 @@ struct BuiltinTypeDeclBuilder {
                                                 ResourceKind RK, bool IsROV) {
     if (Record->isCompleteDefinition())
       return *this;
-    Record->addAttr(HLSLResourceAttr::CreateImplicit(Record->getASTContext(),
-                                                     RC, RK, IsROV));
+    HLSLResourceAttr *attr = HLSLResourceAttr::CreateImplicit(
+        Record->getASTContext(), RC, RK, IsROV);
+
+    Record->addAttr(attr);
     return *this;
   }
 
@@ -482,15 +484,18 @@ static BuiltinTypeDeclBuilder setupBufferType(CXXRecordDecl *Decl, Sema &S,
                                               bool IsROV) {
   return BuiltinTypeDeclBuilder(Decl)
       .addHandleMember()
-      .addDefaultHandleConstructor(S, RC)
-      .annotateResourceClass(RC, RK, IsROV);
+      .addDefaultHandleConstructor(S, RC);
 }
 
 void HLSLExternalSemaSource::defineHLSLTypesWithForwardDeclarations() {
   CXXRecordDecl *Decl;
-  Decl = BuiltinTypeDeclBuilder(*SemaPtr, HLSLNamespace, "RWBuffer")
-             .addSimpleTemplateParams({"element_type"})
-             .Record;
+  Decl =
+      BuiltinTypeDeclBuilder(*SemaPtr, HLSLNamespace, "RWBuffer")
+          .addSimpleTemplateParams({"element_type"})
+          .annotateResourceClass(ResourceClass::UAV, ResourceKind::TypedBuffer,
+                                 /*IsROV=*/false)
+          .Record;
+
   onCompletion(Decl, [this](CXXRecordDecl *Decl) {
     setupBufferType(Decl, *SemaPtr, ResourceClass::UAV,
                     ResourceKind::TypedBuffer, /*IsROV=*/false)
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index f25f3afd0f4af2..e720fe56c3ea17 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -7333,12 +7333,12 @@ static void handleHLSLResourceBindingAttr(Sema &S, Decl *D,
   } else {
     Slot = Str;
   }
-
+  QualType Ty = ((clang::ValueDecl *)D)->getType();
   // Validate.
   if (!Slot.empty()) {
     switch (Slot[0]) {
-    case 'u':
     case 'b':
+    case 'u':
     case 's':
     case 't':
       break;
@@ -7367,6 +7367,75 @@ static void handleHLSLResourceBindingAttr(Sema &S, Decl *D,
     return;
   }
 
+  VarDecl *VD = dyn_cast<VarDecl>(D);
+  HLSLBufferDecl *BD = dyn_cast<HLSLBufferDecl>(D);
+
+  if (VD || BD) {
+    llvm::hlsl::ResourceClass RC;
+    std::string varTy = "";
+    if (VD) {
+
+      const Type *Ty = VD->getType()->getPointeeOrArrayElementType();
+      if (!Ty)
+        return;
+      QualType t = ((ElaboratedType *)Ty)->getNamedType();
+      const CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
+      if (!RD)
+        return;
+
+      if (auto TDecl = dyn_cast<ClassTemplateSpecializationDecl>(RD))
+        RD = TDecl->getSpecializedTemplate()->getTemplatedDecl();
+      RD = RD->getCanonicalDecl();
+      const auto *Attr = RD->getAttr<HLSLResourceAttr>();
+      if (!Attr)
+        return;
+
+      RC = Attr->getResourceClass();
+      varTy = RD->getNameAsString();
+    } else {
+      if (BD->isCBuffer()) {
+        RC = llvm::hlsl::ResourceClass::CBuffer;
+        varTy = "cbuffer";
+      } else {
+        RC = llvm::hlsl::ResourceClass::CBuffer;
+        varTy = "tbuffer";
+      }
+    }
+    switch (RC) {
+    case llvm::hlsl::ResourceClass::SRV: {
+      if (Slot.substr(0, 1) != "t")
+        S.Diag(ArgLoc, diag::err_hlsl_mismatching_register_type_and_name)
+            << Slot.substr(0, 1) << varTy << "t";
+      break;
+    }
+    case llvm::hlsl::ResourceClass::UAV: {
+      if (Slot.substr(0, 1) != "u")
+        S.Diag(ArgLoc, diag::err_hlsl_mismatching_register_type_and_name)
+            << Slot.substr(0, 1) << varTy << "u";
+      break;
+    }
+    case llvm::hlsl::ResourceClass::CBuffer: {
+      if (Slot.substr(0, 1) != "b")
+        S.Diag(ArgLoc, diag::err_hlsl_mismatching_register_type_and_name)
+            << Slot.substr(0, 1) << varTy << "b";
+      break;
+    }
+    case llvm::hlsl::ResourceClass::Sampler: {
+      if (Slot.substr(0, 1) != "s")
+        S.Diag(ArgLoc, diag::err_hlsl_mismatching_register_type_and_name)
+            << Slot.substr(0, 1) << varTy << "s";
+      break;
+    }
+    case llvm::hlsl::ResourceClass::Invalid: {
+      llvm_unreachable("Resource class should be valid.");
+      break;
+    }
+
+    default:
+      break;
+    }
+  }
+
   // FIXME: check reg type match decl. Issue
   // https://github.com/llvm/llvm-project/issues/57886.
   HLSLResourceBindingAttr *NewAttr =
diff --git a/clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl b/clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl
new file mode 100644
index 00000000000000..76dcbd201cbd19
--- /dev/null
+++ b/clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl
@@ -0,0 +1,65 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+
+// expected-error at +1 {{invalid register name prefix 'b' for register type 'RWBuffer' (expected 'u')}}
+RWBuffer<int> a : register(b2, space1);
+
+// expected-error at +1 {{invalid register name prefix 't' for register type 'RWBuffer' (expected 'u')}}
+RWBuffer<int> b : register(t2, space1);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'Texture1D' (expected 't')}}
+// NOT YET IMPLEMENTED Texture1D<float> tex : register(u3);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 's' for register type 'Texture2D' (expected 't')}}
+// NOT YET IMPLEMENTED Texture2D<float> Texture : register(s0);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'Texture2DMS' (expected 't')}}
+// NOT YET IMPLEMENTED Texture2DMS<float4, 4> T2DMS_t2 : register(u2)
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 't' for register type 'RWTexture3D' (expected 'u')}}
+// NOT YET IMPLEMENTED RWTexture3D<float4> RWT3D_u1 : register(t1)
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'b' for register type 'Texture2DMS' (expected 't' or 's')}}
+// NOT YET IMPLEMENTED TextureCube TCube_b2 : register(B2);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'b' for register type 'Texture2DMS' (expected 't')}}
+// NOT YET IMPLEMENTED TextureCubeArray TCubeArray_t2 : register(b2);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'b' for register type 'Texture2DMS' (expected 't' or 's')}}
+// NOT YET IMPLEMENTED Texture1DArray T1DArray_t2 : register(b2);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'Texture2DMS' (expected 't' or 's')}}
+// NOT YET IMPLEMENTED Texture2DArray T2DArray_b2 : register(B2);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'Texture2DMS' (expected 'b' or 'c' or 'i')}}
+// NOT YET IMPLEMENTED Texture2DMSArray<float4> msTextureArray : register(t2, space2);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'TCubeArray_f2' (expected 't' or 's')}}
+// NOT YET IMPLEMENTED TextureCubeArray TCubeArray_f2 : register(u2);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'TypedBuffer' (expected 't')}}
+// NOT YET IMPLEMENTED TypedBuffer tbuf : register(u2);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'RawBuffer' (expected 't')}}
+// NOT YET IMPLEMENTED RawBuffer rbuf : register(u2);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 't' for register type 'StructuredBuffer' (expected 'u')}}
+// NOT YET IMPLEMENTED StructuredBuffer ROVStructuredBuff_t2  : register(T2);
+
+// expected-error at +1 {{invalid register name prefix 's' for register type 'cbuffer' (expected 'b')}}
+cbuffer f : register(s2, space1) {}
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 't' for register type 'Sampler' (expected 's')}}
+// Can this type just be Sampler instead of SamplerState?
+// NOT YET IMPLEMENTED SamplerState MySampler : register(t3, space1);
+
+// expected-error at +1 {{invalid register name prefix 's' for register type 'tbuffer' (expected 'b')}}
+tbuffer f : register(s2, space1) {}
+
+// NOT YET IMPLEMENTED : RTAccelerationStructure doesn't have any example tests in DXC
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'FeedbackTexture2D' (expected 'b' or 'c' or 'i')}}
+// NOT YET IMPLEMENTED FeedbackTexture2D<float> FBTex2D[3][] : register(u0, space26);
+
+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'FeedbackTexture2DArray' (expected 'b' or 'c' or 'i')}}
+// NOT YET IMPLEMENTED FeedbackTexture2DArray<float> FBTex2DArr[3][2][] : register(u0, space27);

>From c307d0e2950976b1862db2a6d3d0005913da4f55 Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Thu, 4 Apr 2024 14:04:58 -0700
Subject: [PATCH 2/4] adjust switch statement, add empty string test cases

---
 clang/lib/Sema/SemaDeclAttr.cpp                   | 15 +++++----------
 .../resource_binding_attr_error_mismatch.hlsl     |  8 ++++++++
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index e720fe56c3ea17..a2b60efbaf5383 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -7403,41 +7403,36 @@ static void handleHLSLResourceBindingAttr(Sema &S, Decl *D,
     }
     switch (RC) {
     case llvm::hlsl::ResourceClass::SRV: {
-      if (Slot.substr(0, 1) != "t")
+      if (Slot[0] != 't')
         S.Diag(ArgLoc, diag::err_hlsl_mismatching_register_type_and_name)
             << Slot.substr(0, 1) << varTy << "t";
       break;
     }
     case llvm::hlsl::ResourceClass::UAV: {
-      if (Slot.substr(0, 1) != "u")
+      if (Slot[0] != 'u')
         S.Diag(ArgLoc, diag::err_hlsl_mismatching_register_type_and_name)
             << Slot.substr(0, 1) << varTy << "u";
       break;
     }
     case llvm::hlsl::ResourceClass::CBuffer: {
-      if (Slot.substr(0, 1) != "b")
+      if (Slot[0] != 'b')
         S.Diag(ArgLoc, diag::err_hlsl_mismatching_register_type_and_name)
             << Slot.substr(0, 1) << varTy << "b";
       break;
     }
     case llvm::hlsl::ResourceClass::Sampler: {
-      if (Slot.substr(0, 1) != "s")
+      if (Slot[0] != 's')
         S.Diag(ArgLoc, diag::err_hlsl_mismatching_register_type_and_name)
             << Slot.substr(0, 1) << varTy << "s";
       break;
     }
-    case llvm::hlsl::ResourceClass::Invalid: {
+    default: {
       llvm_unreachable("Resource class should be valid.");
       break;
     }
-
-    default:
-      break;
     }
   }
 
-  // FIXME: check reg type match decl. Issue
-  // https://github.com/llvm/llvm-project/issues/57886.
   HLSLResourceBindingAttr *NewAttr =
       HLSLResourceBindingAttr::Create(S.getASTContext(), Slot, Space, AL);
   if (NewAttr)
diff --git a/clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl b/clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl
index 76dcbd201cbd19..3bce7a0c290f1a 100644
--- a/clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl
+++ b/clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl
@@ -63,3 +63,11 @@ tbuffer f : register(s2, space1) {}
 
 // NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'FeedbackTexture2DArray' (expected 'b' or 'c' or 'i')}}
 // NOT YET IMPLEMENTED FeedbackTexture2DArray<float> FBTex2DArr[3][2][] : register(u0, space27);
+
+
+// empty binding prefix cases:
+// expected-error at +1 {{expected identifier}}
+RWBuffer<int> c: register();
+
+// expected-error at +1 {{expected identifier}}
+RWBuffer<int> d: register("");

>From 2cde16b4a4a49fecce1bad03b590ba51f2d1fa10 Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Thu, 4 Apr 2024 14:25:36 -0700
Subject: [PATCH 3/4] use invalid case instead of default

---
 clang/lib/Sema/SemaDeclAttr.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index a2b60efbaf5383..01224be123fc58 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -7426,7 +7426,7 @@ static void handleHLSLResourceBindingAttr(Sema &S, Decl *D,
             << Slot.substr(0, 1) << varTy << "s";
       break;
     }
-    default: {
+    case llvm::hlsl::ResourceClass::Invalid: {
       llvm_unreachable("Resource class should be valid.");
       break;
     }

>From c77b3ee1856cf668eb6d430e91abe1c2b6e8ac78 Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Wed, 10 Apr 2024 18:11:47 -0700
Subject: [PATCH 4/4] address part of feedback

---
 clang/lib/Sema/HLSLExternalSemaSource.cpp | 6 ++----
 clang/lib/Sema/SemaDeclAttr.cpp           | 8 ++++----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/clang/lib/Sema/HLSLExternalSemaSource.cpp b/clang/lib/Sema/HLSLExternalSemaSource.cpp
index 479689ec82dcee..fb28fd33d09fd0 100644
--- a/clang/lib/Sema/HLSLExternalSemaSource.cpp
+++ b/clang/lib/Sema/HLSLExternalSemaSource.cpp
@@ -119,10 +119,8 @@ struct BuiltinTypeDeclBuilder {
                                                 ResourceKind RK, bool IsROV) {
     if (Record->isCompleteDefinition())
       return *this;
-    HLSLResourceAttr *attr = HLSLResourceAttr::CreateImplicit(
-        Record->getASTContext(), RC, RK, IsROV);
-
-    Record->addAttr(attr);
+    Record->addAttr(HLSLResourceAttr::CreateImplicit(Record->getASTContext(),
+                                                     RC, RK, IsROV));
     return *this;
   }
 
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 01224be123fc58..596818f43f4723 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -7333,7 +7333,7 @@ static void handleHLSLResourceBindingAttr(Sema &S, Decl *D,
   } else {
     Slot = Str;
   }
-  QualType Ty = ((clang::ValueDecl *)D)->getType();
+
   // Validate.
   if (!Slot.empty()) {
     switch (Slot[0]) {
@@ -7372,13 +7372,13 @@ static void handleHLSLResourceBindingAttr(Sema &S, Decl *D,
 
   if (VD || BD) {
     llvm::hlsl::ResourceClass RC;
-    std::string varTy = "";
+    StringRef varTy = "";
     if (VD) {
 
       const Type *Ty = VD->getType()->getPointeeOrArrayElementType();
       if (!Ty)
         return;
-      QualType t = ((ElaboratedType *)Ty)->getNamedType();
+      QualType t = cast<ElaboratedType>(Ty)->getNamedType();
       const CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
       if (!RD)
         return;
@@ -7391,7 +7391,7 @@ static void handleHLSLResourceBindingAttr(Sema &S, Decl *D,
         return;
 
       RC = Attr->getResourceClass();
-      varTy = RD->getNameAsString();
+      varTy = RD->getName();
     } else {
       if (BD->isCBuffer()) {
         RC = llvm::hlsl::ResourceClass::CBuffer;



More information about the cfe-commits mailing list