[HLSL] Split out resource class data from resource attribute

[bob80905]

The ability to spell out and specify the resource class is necessary for testing various resource binding behaviors. Though it is not intended for users to use this in customized HLSL source code, the ability to specify the resource class via an attribute is immensely helpful for writing thorough tests.
This PR introduces a new attribute, hlsl::resource_attribute, that can only be applied on structs. This attribute only has 1 required argument, and must be one of:

SRV
UAV
CBuffer
Sampler

By applying this attribute to a struct, the struct will have the HLSLResourceClassAttr attribute attached to it in the AST representation, which provides information on the type of resource class the struct is meant to be.

The resource class data that was originally contained within the HLSLResourceAttr attribute has been removed in favor of this new attribute, and so certain ast-dump tests need to be modified so that the same information can be represented via 2 attributes instead of one.

Fixes #98193

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-hlsl

Author: Joshua Batista (bob80905)

Changes

The ability to spell out and specify the resource class is necessary for testing various resource behaviors. Though it is not intended for users to use this in customized HLSL source code, the ability to specify the resource class via an attribute is immensely helpful for writing thorough tests.
This PR introduces a new attribute, hlsl::resource_attribute, that can only be applied on structs. This attribute only has 1 required argument, and must be one of:

SRV
UAV
CBuffer
Sampler

By applying this attribute to a struct, the struct will have the HLSLResourceClassAttr attribute attached to it in the AST representation, which provides information on the type of resource class the struct is meant to be.

Fixes #98193

---

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

10 Files Affected:

• (modified) clang/include/clang/Basic/Attr.td (+14-5)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1)
• (modified) clang/include/clang/Sema/SemaHLSL.h (+1)
• (modified) clang/lib/CodeGen/CGHLSLRuntime.cpp (+7-5)
• (modified) clang/lib/Sema/HLSLExternalSemaSource.cpp (+16-18)
• (modified) clang/lib/Sema/SemaDeclAttr.cpp (+3)
• (modified) clang/lib/Sema/SemaHLSL.cpp (+43)
• (modified) clang/test/AST/HLSL/RWBuffer-AST.hlsl (+4-2)
• (added) clang/test/ParserHLSL/hlsl_resource_class_attr.hlsl (+37)
• (added) clang/test/ParserHLSL/hlsl_resource_class_attr_error.hlsl (+23)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index d2d9dd24536cb..74efff54254c1 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4549,11 +4549,7 @@ def HLSLResource : InheritableAttr {
   let Spellings = [];
   let Subjects = SubjectList<[Struct]>;
   let LangOpts = [HLSL];
-  let Args = [
-    EnumArgument<"ResourceClass", "llvm::hlsl::ResourceClass",
-                 /*is_string=*/0, ["SRV", "UAV", "CBuffer", "Sampler"],
-                 ["SRV", "UAV", "CBuffer", "Sampler"],
-                 /*opt=*/0, /*fake=*/0, /*isExternalType=*/1>,
+  let Args = [    
     EnumArgument<
         "ResourceKind", "llvm::hlsl::ResourceKind",
         /*is_string=*/0,
@@ -4577,6 +4573,19 @@ def HLSLResource : InheritableAttr {
   let Documentation = [InternalOnly];
 }
 
+def HLSLResourceClass : InheritableAttr {
+  let Spellings = [CXX11<"hlsl", "resource_class">];
+  let Subjects = SubjectList<[Struct]>;
+  let LangOpts = [HLSL];
+  let Args = [
+	EnumArgument<"ResourceClass", "llvm::hlsl::ResourceClass",
+				 /*is_string=*/true, ["SRV", "UAV", "CBuffer", "Sampler"],
+				 ["SRV", "UAV", "CBuffer", "Sampler"],
+				 /*opt=*/0, /*fake=*/0, /*isExternalType=*/1>
+  ];
+  let Documentation = [InternalOnly];
+}
+
 def HLSLGroupSharedAddressSpace : TypeAttr {
   let Spellings = [CustomKeyword<"groupshared">];
   let Subjects = SubjectList<[Var]>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 940f9ac226ca8..a460179334edf 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12313,6 +12313,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_unsupported_resource_class : Error<"invalid resource class '%0' used; expected 'SRV', 'UAV', 'CBuffer', or 'Sampler'">;
 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/include/clang/Sema/SemaHLSL.h b/clang/include/clang/Sema/SemaHLSL.h
index 4d6958a1be3e5..2ddbee67c414b 100644
--- a/clang/include/clang/Sema/SemaHLSL.h
+++ b/clang/include/clang/Sema/SemaHLSL.h
@@ -56,6 +56,7 @@ class SemaHLSL : public SemaBase {
   void handleSV_DispatchThreadIDAttr(Decl *D, const ParsedAttr &AL);
   void handlePackOffsetAttr(Decl *D, const ParsedAttr &AL);
   void handleShaderAttr(Decl *D, const ParsedAttr &AL);
+  void handleResourceClassAttr(Decl *D, const ParsedAttr &AL);
   void handleResourceBindingAttr(Decl *D, const ParsedAttr &AL);
   void handleParamModifierAttr(Decl *D, const ParsedAttr &AL);
 
diff --git a/clang/lib/CodeGen/CGHLSLRuntime.cpp b/clang/lib/CodeGen/CGHLSLRuntime.cpp
index 55ba21ae2ba69..9902203d3fae1 100644
--- a/clang/lib/CodeGen/CGHLSLRuntime.cpp
+++ b/clang/lib/CodeGen/CGHLSLRuntime.cpp
@@ -280,13 +280,15 @@ void CGHLSLRuntime::annotateHLSLResource(const VarDecl *D, GlobalVariable *GV) {
   const auto *RD = Ty->getAsCXXRecordDecl();
   if (!RD)
     return;
-  const auto *Attr = RD->getAttr<HLSLResourceAttr>();
-  if (!Attr)
+  const auto *HLSLResAttr = RD->getAttr<HLSLResourceAttr>();
+  const auto *HLSLResClassAttr = RD->getAttr<HLSLResourceClassAttr>();
+  if (!HLSLResAttr || !HLSLResClassAttr)
     return;
 
-  llvm::hlsl::ResourceClass RC = Attr->getResourceClass();
-  llvm::hlsl::ResourceKind RK = Attr->getResourceKind();
-  bool IsROV = Attr->getIsROV();
+  llvm::hlsl::ResourceClass RC = HLSLResClassAttr->getResourceClass();
+  ;
+  llvm::hlsl::ResourceKind RK = HLSLResAttr->getResourceKind();
+  bool IsROV = HLSLResAttr->getIsROV();
   llvm::hlsl::ElementType ET = calculateElementType(CGM.getContext(), Ty);
 
   BufferResBinding Binding(D->getAttr<HLSLResourceBindingAttr>());
diff --git a/clang/lib/Sema/HLSLExternalSemaSource.cpp b/clang/lib/Sema/HLSLExternalSemaSource.cpp
index a2b29a7bdf505..9d715c37436ba 100644
--- a/clang/lib/Sema/HLSLExternalSemaSource.cpp
+++ b/clang/lib/Sema/HLSLExternalSemaSource.cpp
@@ -115,12 +115,19 @@ struct BuiltinTypeDeclBuilder {
     return addMemberVariable("h", Ty, Access);
   }
 
-  BuiltinTypeDeclBuilder &annotateResourceClass(ResourceClass RC,
-                                                ResourceKind RK, bool IsROV) {
+  BuiltinTypeDeclBuilder &annotateHLSLResource(ResourceKind RK, bool IsROV) {
     if (Record->isCompleteDefinition())
       return *this;
-    Record->addAttr(HLSLResourceAttr::CreateImplicit(Record->getASTContext(),
-                                                     RC, RK, IsROV));
+    Record->addAttr(
+        HLSLResourceAttr::CreateImplicit(Record->getASTContext(), RK, IsROV));
+    return *this;
+  }
+
+  BuiltinTypeDeclBuilder &annotateHLSLResourceClass(ResourceClass RC) {
+    if (Record->isCompleteDefinition())
+      return *this;
+    Record->addAttr(
+        HLSLResourceClassAttr::CreateImplicit(Record->getASTContext(), RC));
     return *this;
   }
 
@@ -143,16 +150,7 @@ struct BuiltinTypeDeclBuilder {
                                VD, false, NameInfo, Ty, VK_PRValue);
   }
 
-  static Expr *emitResourceClassExpr(ASTContext &AST, ResourceClass RC) {
-    return IntegerLiteral::Create(
-        AST,
-        llvm::APInt(AST.getIntWidth(AST.UnsignedCharTy),
-                    static_cast<uint8_t>(RC)),
-        AST.UnsignedCharTy, SourceLocation());
-  }
-
-  BuiltinTypeDeclBuilder &addDefaultHandleConstructor(Sema &S,
-                                                      ResourceClass RC) {
+  BuiltinTypeDeclBuilder &addDefaultHandleConstructor(Sema &S) {
     if (Record->isCompleteDefinition())
       return *this;
     ASTContext &AST = Record->getASTContext();
@@ -172,8 +170,7 @@ struct BuiltinTypeDeclBuilder {
     DeclRefExpr *Fn =
         lookupBuiltinFunction(AST, S, "__builtin_hlsl_create_handle");
 
-    Expr *RCExpr = emitResourceClassExpr(AST, RC);
-    Expr *Call = CallExpr::Create(AST, Fn, {RCExpr}, AST.VoidPtrTy, VK_PRValue,
+    Expr *Call = CallExpr::Create(AST, Fn, {}, AST.VoidPtrTy, VK_PRValue,
                                   SourceLocation(), FPOptionsOverride());
 
     CXXThisExpr *This = CXXThisExpr::Create(
@@ -495,8 +492,9 @@ static BuiltinTypeDeclBuilder setupBufferType(CXXRecordDecl *Decl, Sema &S,
                                               bool IsROV) {
   return BuiltinTypeDeclBuilder(Decl)
       .addHandleMember()
-      .addDefaultHandleConstructor(S, RC)
-      .annotateResourceClass(RC, RK, IsROV);
+      .addDefaultHandleConstructor(S)
+      .annotateHLSLResourceClass(RC)
+      .annotateHLSLResource(RK, IsROV);
 }
 
 void HLSLExternalSemaSource::defineHLSLTypesWithForwardDeclarations() {
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index f2cd46d1e7c93..20f46c003a464 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -7053,6 +7053,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
   case ParsedAttr::AT_HLSLResourceBinding:
     S.HLSL().handleResourceBindingAttr(D, AL);
     break;
+  case ParsedAttr::AT_HLSLResourceClass:
+    S.HLSL().handleResourceClassAttr(D, AL);
+    break;
   case ParsedAttr::AT_HLSLParamModifier:
     S.HLSL().handleParamModifierAttr(D, AL);
     break;
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index babb984995f13..2b2f390a01ab1 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -437,6 +437,49 @@ void SemaHLSL::handleShaderAttr(Decl *D, const ParsedAttr &AL) {
     D->addAttr(NewAttr);
 }
 
+llvm::dxil::ResourceClass
+getResourceClassFromStr(StringRef ResourceClassTypeStrRef) {
+  if (ResourceClassTypeStrRef == "SRV")
+    return llvm::dxil::ResourceClass::SRV;
+
+  if (ResourceClassTypeStrRef == "UAV")
+    return llvm::dxil::ResourceClass::UAV;
+
+  if (ResourceClassTypeStrRef == "CBuffer")
+    return llvm::dxil::ResourceClass::CBuffer;
+
+  if (ResourceClassTypeStrRef == "Sampler")
+    return llvm::dxil::ResourceClass::Sampler;
+
+  llvm_unreachable("Unsupported resource class type");
+}
+
+void SemaHLSL::handleResourceClassAttr(Decl *D, const ParsedAttr &AL) {
+  if (!AL.isArgIdent(0)) {
+    Diag(AL.getLoc(), diag::err_attribute_argument_type)
+        << AL << AANT_ArgumentIdentifier;
+    return;
+  }
+
+  IdentifierLoc *Loc = AL.getArgAsIdent(0);
+  StringRef ResourceClassTypeStrRef = Loc->Ident->getName();
+  SourceLocation ArgLoc = Loc->Loc;
+
+  // Validate.
+  if (ResourceClassTypeStrRef != "SRV" && ResourceClassTypeStrRef != "UAV" &&
+      ResourceClassTypeStrRef != "CBuffer" &&
+      ResourceClassTypeStrRef != "Sampler") {
+    Diag(ArgLoc, diag::err_hlsl_unsupported_resource_class)
+        << ResourceClassTypeStrRef;
+    return;
+  }
+
+  HLSLResourceClassAttr *NewAttr = HLSLResourceClassAttr::Create(
+      getASTContext(), getResourceClassFromStr(ResourceClassTypeStrRef));
+  if (NewAttr)
+    D->addAttr(NewAttr);
+}
+
 void SemaHLSL::handleResourceBindingAttr(Decl *D, const ParsedAttr &AL) {
   StringRef Space = "space0";
   StringRef Slot = "";
diff --git a/clang/test/AST/HLSL/RWBuffer-AST.hlsl b/clang/test/AST/HLSL/RWBuffer-AST.hlsl
index ac54194be7f0d..cb66a703a4ec9 100644
--- a/clang/test/AST/HLSL/RWBuffer-AST.hlsl
+++ b/clang/test/AST/HLSL/RWBuffer-AST.hlsl
@@ -34,7 +34,8 @@ RWBuffer<float> Buffer;
 // CHECK-NEXT: CXXRecordDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit class RWBuffer definition
 
 // CHECK: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
-// CHECK-NEXT: HLSLResourceAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit UAV TypedBuffer
+// CHECK-NEXT: HLSLResourceClassAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit UAV
+// CHECK-NEXT: HLSLResourceAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit TypedBuffer
 // CHECK-NEXT: FieldDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> implicit h 'element_type *'
 
 // CHECK: CXXMethodDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> operator[] 'element_type &const (unsigned int) const'
@@ -62,5 +63,6 @@ RWBuffer<float> Buffer;
 // CHECK: TemplateArgument type 'float'
 // CHECK-NEXT: BuiltinType 0x{{[0-9A-Fa-f]+}} 'float'
 // CHECK-NEXT: FinalAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit final
-// CHECK-NEXT: HLSLResourceAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit UAV TypedBuffer
+// CHECK-NEXT: HLSLResourceClassAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit UAV
+// CHECK-NEXT: HLSLResourceAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit TypedBuffer
 // CHECK-NEXT: FieldDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc>  implicit referenced h 'float *'
diff --git a/clang/test/ParserHLSL/hlsl_resource_class_attr.hlsl b/clang/test/ParserHLSL/hlsl_resource_class_attr.hlsl
new file mode 100644
index 0000000000000..3ca03ba1a488b
--- /dev/null
+++ b/clang/test/ParserHLSL/hlsl_resource_class_attr.hlsl
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -ast-dump -o - %s | FileCheck %s
+
+// CHECK: -HLSLResourceClassAttr 0x{{[0-9a-f]+}} <<invalid sloc>> SRV
+struct [[hlsl::resource_class(SRV)]] Eg1 {
+  int i;  
+};
+
+Eg1 e1;
+
+// CHECK: -CXXRecordDecl 0x{{[0-9a-f]+}} <line:13:1, line:15:1> line:13:38 referenced struct Eg2 definition
+// CHECK: -HLSLResourceClassAttr 0x{{[0-9a-f]+}} <<invalid sloc>> UAV
+struct [[hlsl::resource_class(UAV)]] Eg2 {
+  int i;
+};
+Eg2 e2;
+
+// CHECK: -CXXRecordDecl 0x{{[0-9a-f]+}} <line:20:1, line:22:1> line:20:42 referenced struct Eg3 definition
+// CHECK: -HLSLResourceClassAttr 0x{{[0-9a-f]+}} <<invalid sloc>> CBuffer
+struct [[hlsl::resource_class(CBuffer)]] Eg3 {
+  int i;
+}; 
+Eg3 e3;
+
+// CHECK: -CXXRecordDecl 0x{{[0-9a-f]+}} <line:27:1, line:29:1> line:27:42 referenced struct Eg4 definition
+// CHECK: -HLSLResourceClassAttr 0x{{[0-9a-f]+}} <<invalid sloc>> Sampler
+struct [[hlsl::resource_class(Sampler)]] Eg4 {
+  int i;
+};
+Eg4 e4;
+
+RWBuffer<int> In : register(u1);
+
+
+[numthreads(1,1,1)]
+void main() {
+  In[0] = e1.i + e2.i + e3.i + e4.i;
+}
diff --git a/clang/test/ParserHLSL/hlsl_resource_class_attr_error.hlsl b/clang/test/ParserHLSL/hlsl_resource_class_attr_error.hlsl
new file mode 100644
index 0000000000000..463c746711d4a
--- /dev/null
+++ b/clang/test/ParserHLSL/hlsl_resource_class_attr_error.hlsl
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -o - %s -verify
+
+// expected-error@+1{{'resource_class' attribute takes one argument}}
+struct [[hlsl::resource_class()]] Eg1 {
+  int i;  
+};
+
+Eg1 e1;
+
+// expected-error@+1{{invalid resource class 'gibberish' used; expected 'SRV', 'UAV', 'CBuffer', or 'Sampler'}}
+struct [[hlsl::resource_class(gibberish)]] Eg2 {
+  int i;  
+};
+
+Eg2 e2;
+
+RWBuffer<int> In : register(u1);
+
+
+[numthreads(1,1,1)]
+void main() {
+  In[0] = e1.i + e2.i;
+}

[bogner]

You should be able to use HLSLResourceClassAttr::ConvertStrToResourceClass here, rather than hand rolling this. It's also probably fine to just emit diag::warn_attribute_type_not_supported rather than create a more specific message just for this.

[bogner]

I'm surprised this has an invalid source location given that it's coming from the source. Did you use the right constructor for the attribute?

[bob80905]

Added ArgLoc to the constructor, it was an optional arg

[damyanp]

I think this change will make ResourceTypes.rst be out of date. Should we fix the documentation up as we go?

[bogner]

This reads more simply if you just test the result of the function in the if condition. Also, you probably don't need quite that verbose of a name for the identifier, so I'd probably write this like:

  if (!HLSLResourceClassAttr::ConvertStrToResourceClass(Identifier, RC)) {

[bogner]

Can HLSLResourceClassAttr::Create(...) fail? Does it actually make sense to check if NewAttr is null here?

[bob80905]

I don't think it can be null on further inspection, though I did copy this code form the resource binding attr block. I'll remove this unneeded if check.