Implement resource binding type prefix mismatch errors #87578

bob80905 · 2024-04-03T22:54:32Z

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 · 2024-04-03T22:55:10Z

@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);

clang/lib/Sema/SemaDeclAttr.cpp

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/Sema/HLSLExternalSemaSource.cpp

clang/lib/Sema/SemaDeclAttr.cpp

clang/test/CodeGenHLSL/cbuf.hlsl

clang/lib/Sema/HLSLExternalSemaSource.cpp

clang/lib/Sema/SemaDeclAttr.cpp

clang/test/SemaHLSL/resource_binding_attr_error.hlsl

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl

clang/lib/Sema/SemaDeclAttr.cpp

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl

clang/include/clang/Basic/DiagnosticSemaKinds.td

llvm/include/llvm/Frontend/HLSL/HLSLResource.h

clang/test/SemaHLSL/resource_binding_attr_error.hlsl

clang/lib/Sema/SemaDeclAttr.cpp

…llow builtin types as resource types

damyanp · 2024-05-02T19:00:20Z

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl

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


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.

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl

damyanp · 2024-05-02T19:01:59Z

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl

+// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'Texture2DMSArray' (expected 't')}}
+// NOT YET IMPLEMENTED Texture2DMSArray<float4> msTextureArray : register(t2, space2);


Error message also doesn't match the register in this case.

python3kgae · 2024-05-02T19:45:09Z

clang/test/AST/HLSL/resource_binding_attr.hlsl

@@ -1,13 +1,15 @@
 // RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -ast-dump -o - %s | FileCheck %s



Should we support this?

float c : register(c10);

It is legal in dxc.

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

Co-authored-by: Damyan Pepper <damyanp@microsoft.com>

implement binding type error for t/cbuffers and rwbuffers

3960050

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels Apr 3, 2024

Keenuts reviewed Apr 4, 2024

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

python3kgae reviewed Apr 4, 2024

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

python3kgae reviewed Apr 4, 2024

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

python3kgae reviewed Apr 4, 2024

View reviewed changes

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl Show resolved Hide resolved

adjust switch statement, add empty string test cases

c307d0e

llvm-beanz reviewed Apr 4, 2024

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

use invalid case instead of default

2cde16b

bogner reviewed Apr 10, 2024

View reviewed changes

address part of feedback

c77b3ee

farzonl reviewed Apr 15, 2024

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

bob80905 added 4 commits April 17, 2024 19:45

address rest of feedback, rename variables, create new function

110eef6

Merge branch 'main' into validate_register_binding_hlsl

23e893a

remove unneeded function

1a9ea6c

format

f985ab6

llvmbot added the clang:modules C++20 modules and Clang Header Modules label Apr 22, 2024

python3kgae reviewed Apr 22, 2024

View reviewed changes

clang/test/CodeGenHLSL/cbuf.hlsl Outdated Show resolved Hide resolved

bogner reviewed Apr 22, 2024

View reviewed changes

tex3d reviewed Apr 23, 2024

View reviewed changes

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl Outdated Show resolved Hide resolved

tex3d reviewed Apr 23, 2024

View reviewed changes

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl Outdated Show resolved Hide resolved

tex3d reviewed Apr 23, 2024

View reviewed changes

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl Outdated Show resolved Hide resolved

bob80905 added 2 commits April 23, 2024 16:00

address all of Justin and Xiang's feedback

3b2afe7

address tex

dcaf3ad

bogner reviewed Apr 24, 2024

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Show resolved Hide resolved

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl Outdated Show resolved Hide resolved

python3kgae reviewed Apr 24, 2024

View reviewed changes

clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved

clean up switch statement, remove bad comments

1edee6e

python3kgae reviewed Apr 24, 2024

View reviewed changes

llvm/include/llvm/Frontend/HLSL/HLSLResource.h Outdated Show resolved Hide resolved

remove tbuffer as a resource class

9bdcea2

python3kgae reviewed Apr 29, 2024

View reviewed changes

clang/test/SemaHLSL/resource_binding_attr_error.hlsl Outdated Show resolved Hide resolved

bogner reviewed Apr 29, 2024

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaDeclAttr.cpp Show resolved Hide resolved

add resource attr to hlslbufferdecl, drive diags from attr. Also disa…

8baae27

…llow builtin types as resource types

damyanp reviewed May 2, 2024

View reviewed changes

clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl Outdated Show resolved Hide resolved

damyanp reviewed May 2, 2024

View reviewed changes

python3kgae reviewed May 2, 2024

View reviewed changes

Update clang/test/SemaHLSL/resource_binding_attr_error_mismatch.hlsl

80d7209

Co-authored-by: Damyan Pepper <damyanp@microsoft.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement resource binding type prefix mismatch errors #87578

Implement resource binding type prefix mismatch errors #87578

bob80905 commented Apr 3, 2024

llvmbot commented Apr 3, 2024 •

edited

damyanp May 2, 2024

damyanp May 2, 2024

python3kgae May 2, 2024 •

edited

damyanp May 2, 2024 •

edited



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

		// NOT YET IMPLEMENTED : {{invalid register name prefix 'u' for register type 'Texture2DMSArray' (expected 't')}}
		// NOT YET IMPLEMENTED Texture2DMSArray<float4> msTextureArray : register(t2, space2);

		@@ -1,13 +1,15 @@
		// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -ast-dump -o - %s \| FileCheck %s

Implement resource binding type prefix mismatch errors #87578

Are you sure you want to change the base?

Implement resource binding type prefix mismatch errors #87578

Conversation

bob80905 commented Apr 3, 2024

llvmbot commented Apr 3, 2024 • edited

damyanp May 2, 2024

Choose a reason for hiding this comment

damyanp May 2, 2024

Choose a reason for hiding this comment

python3kgae May 2, 2024 • edited

Choose a reason for hiding this comment

damyanp May 2, 2024 • edited

Choose a reason for hiding this comment

llvmbot commented Apr 3, 2024 •

edited

python3kgae May 2, 2024 •

edited

damyanp May 2, 2024 •

edited