Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[HLSL] Split out resource class data from resource attribute #98419

Merged
merged 8 commits into from
Jul 12, 2024
+105 −18
Merged

[HLSL] Split out resource class data from resource attribute #98419

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b740aa9
split out resource class data from resource attr, add some prelim dia…
bob80905 Jul 11, 2024
d5d4b00
add back ResourceClass for default handle constructor for codegen tests
bob80905 Jul 11, 2024
8b12c39
add hlslresourceclassattr to attr list test
bob80905 Jul 11, 2024
65da73a
Update clang/lib/CodeGen/CGHLSLRuntime.cpp
bob80905 Jul 11, 2024
9cf74db
shorten tests, include sloc in attr construction, use existing RC val…
bob80905 Jul 11, 2024
a9a02c4
remove unneeded error, update test
bob80905 Jul 11, 2024
30c3b4c
remove unneeded check, simplify verbose var name, use extra sloc arg …
bob80905 Jul 11, 2024
2539a08
remove entry points and make tests dump ast
bob80905 Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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]>;
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'">;
damyanp marked this conversation as resolved.
Show resolved Hide resolved
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">;
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Sema/SemaHLSL.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
12 changes: 7 additions & 5 deletions clang/lib/CodeGen/CGHLSLRuntime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
;
damyanp marked this conversation as resolved.
Show resolved Hide resolved
llvm::hlsl::ResourceKind RK = HLSLResAttr->getResourceKind();
bool IsROV = HLSLResAttr->getIsROV();
llvm::hlsl::ElementType ET = calculateElementType(CGM.getContext(), Ty);

BufferResBinding Binding(D->getAttr<HLSLResourceBindingAttr>());
Expand Down
34 changes: 16 additions & 18 deletions clang/lib/Sema/HLSLExternalSemaSource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
bob80905 marked this conversation as resolved.
Show resolved Hide resolved
return *this;
}

Expand All @@ -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();
Expand All @@ -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(
Expand Down Expand Up @@ -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() {
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
43 changes: 43 additions & 0 deletions clang/lib/Sema/SemaHLSL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


HLSLResourceClassAttr *NewAttr = HLSLResourceClassAttr::Create(
getASTContext(), getResourceClassFromStr(ResourceClassTypeStrRef));
damyanp marked this conversation as resolved.
Show resolved Hide resolved
if (NewAttr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

D->addAttr(NewAttr);
}

void SemaHLSL::handleResourceBindingAttr(Decl *D, const ParsedAttr &AL) {
StringRef Space = "space0";
StringRef Slot = "";
Expand Down
6 changes: 4 additions & 2 deletions clang/test/AST/HLSL/RWBuffer-AST.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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 *'
37 changes: 37 additions & 0 deletions clang/test/ParserHLSL/hlsl_resource_class_attr.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
damyanp marked this conversation as resolved.
Show resolved Hide resolved
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added ArgLoc to the constructor, it was an optional arg

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;
}
23 changes: 23 additions & 0 deletions clang/test/ParserHLSL/hlsl_resource_class_attr_error.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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'}}
damyanp marked this conversation as resolved.
Show resolved Hide resolved
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;
}
damyanp marked this conversation as resolved.
Show resolved Hide resolved
Loading