[HLSL] Adjust resource binding diagnostic flags code #106657
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-hlsl @llvm/pr-subscribers-clang Author: Helena Kotas (hekota) ChangesAdjust register binding diagnostic flags code in a couple of ways:
Also fixes a case where struct with an array was incorrectly diagnosed unfit for This will also simplify work that is needed to be done in this area for llvm/llvm-project#104861. Full diff: https://github.com/llvm/llvm-project/pull/106657.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index 714e8f5cfa9926..1e484f754b931d 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -480,6 +480,9 @@ struct RegisterBindingFlags {
bool ContainsNumeric = false;
bool DefaultGlobals = false;
+
+ // used only when Resource == true
+ llvm::dxil::ResourceClass ResourceClass = llvm::dxil::ResourceClass::UAV;
};
static bool isDeclaredWithinCOrTBuffer(const Decl *TheDecl) {
@@ -545,65 +548,38 @@ static const T *getSpecifiedHLSLAttrFromVarDecl(VarDecl *VD) {
return getSpecifiedHLSLAttrFromRecordDecl<T>(TheRecordDecl);
}
-static void updateFlagsFromType(QualType TheQualTy,
- RegisterBindingFlags &Flags);
-
-static void updateResourceClassFlagsFromRecordDecl(RegisterBindingFlags &Flags,
- const RecordDecl *RD) {
- if (!RD)
- return;
-
- if (RD->isCompleteDefinition()) {
- for (auto Field : RD->fields()) {
- QualType T = Field->getType();
- updateFlagsFromType(T, Flags);
+static void updateResourceClassFlagsFromRecordType(RegisterBindingFlags &Flags,
+ const RecordType *RT) {
+ llvm::SmallVector<const Type *> TypesToScan;
+ TypesToScan.emplace_back(RT);
+
+ while (!TypesToScan.empty()) {
+ const Type *T = TypesToScan.pop_back_val();
+ while (T->isArrayType())
+ T = T->getArrayElementTypeNoTypeQual();
+ if (T->isIntegralOrEnumerationType() || T->isFloatingType()) {
+ Flags.ContainsNumeric = true;
+ continue;
}
- }
-}
-
-static void updateFlagsFromType(QualType TheQualTy,
- RegisterBindingFlags &Flags) {
- // if the member's type is a numeric type, set the ContainsNumeric flag
- if (TheQualTy->isIntegralOrEnumerationType() || TheQualTy->isFloatingType()) {
- Flags.ContainsNumeric = true;
- return;
- }
-
- const clang::Type *TheBaseType = TheQualTy.getTypePtr();
- while (TheBaseType->isArrayType())
- TheBaseType = TheBaseType->getArrayElementTypeNoTypeQual();
- // otherwise, if the member's base type is not a record type, return
- const RecordType *TheRecordTy = TheBaseType->getAs<RecordType>();
- if (!TheRecordTy)
- return;
-
- RecordDecl *SubRecordDecl = TheRecordTy->getDecl();
- const HLSLResourceClassAttr *Attr =
- getSpecifiedHLSLAttrFromRecordDecl<HLSLResourceClassAttr>(SubRecordDecl);
- // find the attr if it's on the member, or on any of the member's fields
- if (Attr) {
- llvm::hlsl::ResourceClass DeclResourceClass = Attr->getResourceClass();
- updateResourceClassFlagsFromDeclResourceClass(Flags, DeclResourceClass);
- }
+ const RecordType *RT = T->getAs<RecordType>();
+ if (!RT)
+ continue;
- // otherwise, dig deeper and recurse into the member
- else {
- updateResourceClassFlagsFromRecordDecl(Flags, SubRecordDecl);
+ const RecordDecl *RD = RT->getDecl();
+ for (FieldDecl *FD : RD->fields()) {
+ if (HLSLResourceClassAttr *RCAttr =
+ FD->getAttr<HLSLResourceClassAttr>()) {
+ updateResourceClassFlagsFromDeclResourceClass(
+ Flags, RCAttr->getResourceClass());
+ continue;
+ }
+ TypesToScan.emplace_back(FD->getType().getTypePtr());
+ }
}
}
static RegisterBindingFlags HLSLFillRegisterBindingFlags(Sema &S,
Decl *TheDecl) {
-
- // Cbuffers and Tbuffers are HLSLBufferDecl types
- HLSLBufferDecl *CBufferOrTBuffer = dyn_cast<HLSLBufferDecl>(TheDecl);
- // Samplers, UAVs, and SRVs are VarDecl types
- VarDecl *TheVarDecl = dyn_cast<VarDecl>(TheDecl);
-
- assert(((TheVarDecl && !CBufferOrTBuffer) ||
- (!TheVarDecl && CBufferOrTBuffer)) &&
- "either TheVarDecl or CBufferOrTBuffer should be set");
-
RegisterBindingFlags Flags;
// check if the decl type is groupshared
@@ -612,57 +588,61 @@ static RegisterBindingFlags HLSLFillRegisterBindingFlags(Sema &S,
return Flags;
}
- if (!isDeclaredWithinCOrTBuffer(TheDecl)) {
- // make sure the type is a basic / numeric type
- if (TheVarDecl) {
- QualType TheQualTy = TheVarDecl->getType();
- // a numeric variable or an array of numeric variables
- // will inevitably end up in $Globals buffer
- const clang::Type *TheBaseType = TheQualTy.getTypePtr();
- while (TheBaseType->isArrayType())
- TheBaseType = TheBaseType->getArrayElementTypeNoTypeQual();
- if (TheBaseType->isIntegralType(S.getASTContext()) ||
- TheBaseType->isFloatingType())
- Flags.DefaultGlobals = true;
- }
- }
-
- if (CBufferOrTBuffer) {
+ // Cbuffers and Tbuffers are HLSLBufferDecl types
+ if (HLSLBufferDecl *CBufferOrTBuffer = dyn_cast<HLSLBufferDecl>(TheDecl)) {
Flags.Resource = true;
- if (CBufferOrTBuffer->isCBuffer())
- Flags.CBV = true;
- else
- Flags.SRV = true;
- } else if (TheVarDecl) {
+ Flags.ResourceClass = CBufferOrTBuffer->isCBuffer()
+ ? llvm::dxil::ResourceClass::CBuffer
+ : llvm::dxil::ResourceClass::SRV;
+ }
+ // Samplers, UAVs, and SRVs are VarDecl types
+ else if (VarDecl *TheVarDecl = dyn_cast<VarDecl>(TheDecl)) {
const HLSLResourceClassAttr *resClassAttr =
getSpecifiedHLSLAttrFromVarDecl<HLSLResourceClassAttr>(TheVarDecl);
-
if (resClassAttr) {
- llvm::hlsl::ResourceClass DeclResourceClass =
- resClassAttr->getResourceClass();
Flags.Resource = true;
- updateResourceClassFlagsFromDeclResourceClass(Flags, DeclResourceClass);
+ Flags.ResourceClass = resClassAttr->getResourceClass();
} else {
const clang::Type *TheBaseType = TheVarDecl->getType().getTypePtr();
while (TheBaseType->isArrayType())
TheBaseType = TheBaseType->getArrayElementTypeNoTypeQual();
- if (TheBaseType->isArithmeticType())
+
+ if (TheBaseType->isArithmeticType()) {
Flags.Basic = true;
- else if (TheBaseType->isRecordType()) {
+ if (!isDeclaredWithinCOrTBuffer(TheDecl) &&
+ (TheBaseType->isIntegralType(S.getASTContext()) ||
+ TheBaseType->isFloatingType()))
+ Flags.DefaultGlobals = true;
+ } else if (TheBaseType->isRecordType()) {
Flags.UDT = true;
const RecordType *TheRecordTy = TheBaseType->getAs<RecordType>();
- assert(TheRecordTy && "The Qual Type should be Record Type");
- const RecordDecl *TheRecordDecl = TheRecordTy->getDecl();
- // recurse through members, set appropriate resource class flags.
- updateResourceClassFlagsFromRecordDecl(Flags, TheRecordDecl);
+ updateResourceClassFlagsFromRecordType(Flags, TheRecordTy);
} else
Flags.Other = true;
}
+ } else {
+ llvm_unreachable("expected be VarDecl or HLSLBufferDecl");
}
return Flags;
}
-enum class RegisterType { SRV, UAV, CBuffer, Sampler, C, I, Invalid };
+enum class RegisterType {
+ SRV = static_cast<int>(llvm::dxil::ResourceClass::SRV),
+ UAV = static_cast<int>(llvm::dxil::ResourceClass::UAV),
+ CBuffer = static_cast<int>(llvm::dxil::ResourceClass::CBuffer),
+ Sampler = static_cast<int>(llvm::dxil::ResourceClass::Sampler),
+ C,
+ I,
+ Invalid
+};
+
+static RegisterType
+convertResourceClassToRegisterType(llvm::dxil::ResourceClass RC) {
+ assert(RC >= llvm::dxil::ResourceClass::SRV &&
+ RC <= llvm::dxil::ResourceClass::Sampler &&
+ "unexpected resource class value");
+ return static_cast<RegisterType>(RC);
+}
static RegisterType getRegisterType(StringRef Slot) {
switch (Slot[0]) {
@@ -754,34 +734,9 @@ static void DiagnoseHLSLRegisterAttribute(Sema &S, SourceLocation &ArgLoc,
// next, if resource is set, make sure the register type in the register
// annotation is compatible with the variable's resource type.
if (Flags.Resource) {
- const HLSLResourceClassAttr *resClassAttr = nullptr;
- if (CBufferOrTBuffer) {
- resClassAttr = CBufferOrTBuffer->getAttr<HLSLResourceClassAttr>();
- } else if (TheVarDecl) {
- resClassAttr =
- getSpecifiedHLSLAttrFromVarDecl<HLSLResourceClassAttr>(TheVarDecl);
- }
-
- assert(resClassAttr &&
- "any decl that set the resource flag on analysis should "
- "have a resource class attribute attached.");
- const llvm::hlsl::ResourceClass DeclResourceClass =
- resClassAttr->getResourceClass();
-
- // confirm that the register type is bound to its expected resource class
- static RegisterType ExpectedRegisterTypesForResourceClass[] = {
- RegisterType::SRV,
- RegisterType::UAV,
- RegisterType::CBuffer,
- RegisterType::Sampler,
- };
- assert((size_t)DeclResourceClass <
- std::size(ExpectedRegisterTypesForResourceClass) &&
- "DeclResourceClass has unexpected value");
-
- RegisterType ExpectedRegisterType =
- ExpectedRegisterTypesForResourceClass[(int)DeclResourceClass];
- if (regType != ExpectedRegisterType) {
+ RegisterType expRegType =
+ convertResourceClassToRegisterType(Flags.ResourceClass);
+ if (regType != expRegType) {
S.Diag(TheDecl->getLocation(), diag::err_hlsl_binding_type_mismatch)
<< regTypeNum;
}
@@ -823,7 +778,7 @@ static void DiagnoseHLSLRegisterAttribute(Sema &S, SourceLocation &ArgLoc,
}
void SemaHLSL::handleResourceBindingAttr(Decl *TheDecl, const ParsedAttr &AL) {
- if (dyn_cast<VarDecl>(TheDecl)) {
+ if (isa<VarDecl>(TheDecl)) {
if (SemaRef.RequireCompleteType(TheDecl->getBeginLoc(),
cast<ValueDecl>(TheDecl)->getType(),
diag::err_incomplete_type))
diff --git a/clang/test/SemaHLSL/resource_binding_attr_error_udt.hlsl b/clang/test/SemaHLSL/resource_binding_attr_error_udt.hlsl
index f8e38b6d2851d9..edb3f30739cdfd 100644
--- a/clang/test/SemaHLSL/resource_binding_attr_error_udt.hlsl
+++ b/clang/test/SemaHLSL/resource_binding_attr_error_udt.hlsl
@@ -126,3 +126,10 @@ struct Eg14{
};
// expected-warning@+1{{binding type 't' only applies to types containing SRV resources}}
Eg14 e14 : register(t9);
+
+struct Eg15 {
+ float f[4];
+};
+// expected no error
+Eg15 e15 : register(c0);
+
|
damyanp
approved these changes
Aug 30, 2024
clang/lib/Sema/SemaHLSL.cpp
Outdated
| @@ -480,6 +480,9 @@ struct RegisterBindingFlags { | |||
|
|
|||
| bool ContainsNumeric = false; | |||
| bool DefaultGlobals = false; | |||
|
|
|||
| // used only when Resource == true | |||
| llvm::dxil::ResourceClass ResourceClass = llvm::dxil::ResourceClass::UAV; | |||
There was a problem hiding this comment.
What if this was std::optional<llvm::dxil::ResourceClass>? Then you wouldn't need the Resource flag, and you wouldn't need to initialize ResourceClass to some random value.
There was a problem hiding this comment.
Along with this, the various SRV, UAV, CBV, Sampler flags now seem redundant with ResourceClass. Are these flags only valid when UDR is set?
If that's the case then I wonder if this might be better expressed as a std::variant?
There was a problem hiding this comment.
Yes, the individual SRV, UAV, CBV, Sampler flags are used only when UDT is set, and the ResourceClass is only used when Resource is set. I will update ResourceClass to use <srd::optional> to avoid the explicit initialization, but if that's ok with you I'd like to leave the rest as is.
The reason is that I have another change in mind that would remove the need to have the Flags struct in the first place. We only need to make sure there is a resource that matches the provided register type, so we might as well embed the diagnostics right where we are currently setting the flags without building up the struct. And we can bail out early if we find a matching resource class without needing to scan the whole type.
bogner
approved these changes
Sep 3, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/4614 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/4830 Here is the relevant piece of the build log for the reference |
|
I've fixed the warnings from this PR with b2dabd2. Now, I'm not familiar with the code here, but is it still important to do the consistency check on these two variables if you don't use them in |
|
Thank you @kazutakahirata! I will remove the code; the consistency check is repeated later in a called function. |
hekota
added a commit
that referenced
this pull request
Sep 5, 2024
Changes the assert to test the same condition without using the variables. This change is done in response to a comment [here](#106657 (comment)).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adjust register binding diagnostic flags code in a couple of ways:
Also fixes a case where struct with an array was incorrectly diagnosed unfit for
cregister binding.This will also simplify work that is needed to be done in this area for #104861.