Implement resource binding type prefix mismatch errors

[bob80905]

There are currently no diagnostics being emitted for when a resource is bound to a register with an incorrect binding type prefix. For example, a CBuffer type resource should be bound with a a binding type prefix of 'b', but if instead the prefix is 'u', no errors will be emitted. This PR implements a set of diagnostics for when the prefix is not expected.
Fixes #57886

[llvmbot]

@llvm/pr-subscribers-clang-modules
@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Joshua Batista (bob80905)

Changes

There are currently no diagnostics being emitted for when a resource is bound to a register with an incorrect binding type prefix. For example, a CBuffer type resource should be bound with a a binding type prefix of 'b', but if instead the prefix is 'u', no errors will be emitted. This PR implements a set of diagnostics for when the prefix is not expected.
Fixes #57886

---

Full diff: https://github.com/llvm/llvm-project/pull/87578.diff

4 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1)
• (modified) clang/lib/Sema/HLSLExternalSemaSource.cpp (+12-7)
• (modified) clang/lib/Sema/SemaDeclAttr.cpp (+71-2)
• (added) clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl (+65)

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@+1 {{invalid register name prefix 'b' for register type 'RWBuffer' (expected 'u')}}
+RWBuffer<int> a : register(b2, space1);
+
+// expected-error@+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@+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@+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);

[damyanp]

float b : register(u0, space1);

This would also be invalid even if the register type were correct because you can't have spaces on global constants.

[python3kgae]

Should we support this?

 float c : register(c10);

It is legal in dxc.

[damyanp]

yes (unsure why we would consider not supporting it?)

[bob80905]

This PR was based on code from April 3, and the branch is far too stale to update at this point. I'm closing this, and have moved the implementation over to this PR:
#97103