Skip to content

[HLSL][RootSignature] Retain SourceLocation of RootElement for SemaHLSL diagnostics #147094

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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -13079,6 +13079,7 @@ def err_hlsl_resource_range_overlap: Error<
"resource ranges %sub{subst_hlsl_format_ranges}0,1,2,3 and %sub{subst_hlsl_format_ranges}4,5,6,7 "
"overlap within space = %8 and visibility = "
"%select{All|Vertex|Hull|Domain|Geometry|Pixel|Amplification|Mesh}9">;
def note_hlsl_resource_range_here: Note<"overlapping resource range here">;

// Layout randomization diagnostics.
def err_non_designated_init_used : Error<
Expand Down
6 changes: 4 additions & 2 deletions clang/include/clang/Parse/ParseHLSLRootSignature.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "clang/Basic/DiagnosticParse.h"
#include "clang/Lex/LexHLSLRootSignature.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Sema/SemaHLSL.h"

#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
Expand All @@ -29,7 +30,7 @@ namespace hlsl {
class RootSignatureParser {
public:
RootSignatureParser(llvm::dxbc::RootSignatureVersion Version,
SmallVector<llvm::hlsl::rootsig::RootElement> &Elements,
SmallVector<RootSignatureElement> &Elements,
StringLiteral *Signature, Preprocessor &PP);

/// Consumes tokens from the Lexer and constructs the in-memory
Expand Down Expand Up @@ -196,7 +197,8 @@ class RootSignatureParser {

private:
llvm::dxbc::RootSignatureVersion Version;
SmallVector<llvm::hlsl::rootsig::RootElement> &Elements;
SmallVector<RootSignatureElement> &Elements;

clang::StringLiteral *Signature;
RootSignatureLexer Lexer;
clang::Preprocessor &PP;
Expand Down
29 changes: 25 additions & 4 deletions clang/include/clang/Sema/SemaHLSL.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,25 @@ class ParsedAttr;
class Scope;
class VarDecl;

namespace hlsl {

// Introduce a wrapper struct around the underlying RootElement. This structure
// will retain extra clang diagnostic information that is not available in llvm.
struct RootSignatureElement {
RootSignatureElement(SourceLocation Loc,
llvm::hlsl::rootsig::RootElement Element)
: Loc(Loc), Element(Element) {}

const llvm::hlsl::rootsig::RootElement &getElement() const { return Element; }
const SourceLocation &getLocation() const { return Loc; }

private:
SourceLocation Loc;
llvm::hlsl::rootsig::RootElement Element;
};

} // namespace hlsl

using llvm::dxil::ResourceClass;

// FIXME: This can be hidden (as static function in SemaHLSL.cpp) once we no
Expand Down Expand Up @@ -130,12 +149,14 @@ class SemaHLSL : public SemaBase {

/// Creates the Root Signature decl of the parsed Root Signature elements
/// onto the AST and push it onto current Scope
void ActOnFinishRootSignatureDecl(
SourceLocation Loc, IdentifierInfo *DeclIdent,
SmallVector<llvm::hlsl::rootsig::RootElement> &Elements);
void
ActOnFinishRootSignatureDecl(SourceLocation Loc, IdentifierInfo *DeclIdent,
ArrayRef<hlsl::RootSignatureElement> Elements);

// Returns true when D is invalid and a diagnostic was produced
bool handleRootSignatureDecl(HLSLRootSignatureDecl *D, SourceLocation Loc);
bool
handleRootSignatureElements(ArrayRef<hlsl::RootSignatureElement> Elements,
SourceLocation Loc);
void handleRootSignatureAttr(Decl *D, const ParsedAttr &AL);
void handleNumThreadsAttr(Decl *D, const ParsedAttr &AL);
void handleWaveSizeAttr(Decl *D, const ParsedAttr &AL);
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Parse/ParseDeclCXX.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4951,7 +4951,7 @@ void Parser::ParseHLSLRootSignatureAttributeArgs(ParsedAttributes &Attrs) {
// signature string and construct the in-memory elements
if (!Found) {
// Invoke the root signature parser to construct the in-memory constructs
SmallVector<llvm::hlsl::rootsig::RootElement> RootElements;
SmallVector<hlsl::RootSignatureElement> RootElements;
hlsl::RootSignatureParser Parser(getLangOpts().HLSLRootSigVer, RootElements,
Signature, PP);
if (Parser.parse()) {
Expand Down
27 changes: 19 additions & 8 deletions clang/lib/Parse/ParseHLSLRootSignature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,49 +19,59 @@ using TokenKind = RootSignatureToken::Kind;

RootSignatureParser::RootSignatureParser(
llvm::dxbc::RootSignatureVersion Version,
SmallVector<RootElement> &Elements, StringLiteral *Signature,
SmallVector<RootSignatureElement> &Elements, StringLiteral *Signature,
Preprocessor &PP)
: Version(Version), Elements(Elements), Signature(Signature),
Lexer(Signature->getString()), PP(PP), CurToken(0) {}

bool RootSignatureParser::parse() {
// Iterate as many RootElements as possible
// Iterate as many RootSignatureElements as possible
do {
std::optional<RootSignatureElement> Element = std::nullopt;
if (tryConsumeExpectedToken(TokenKind::kw_RootFlags)) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Flags = parseRootFlags();
if (!Flags.has_value())
return true;
Elements.push_back(*Flags);
Element = RootSignatureElement(ElementLoc, *Flags);
}

if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Constants = parseRootConstants();
if (!Constants.has_value())
return true;
Elements.push_back(*Constants);
Element = RootSignatureElement(ElementLoc, *Constants);
}

if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Table = parseDescriptorTable();
if (!Table.has_value())
return true;
Elements.push_back(*Table);
Element = RootSignatureElement(ElementLoc, *Table);
}

if (tryConsumeExpectedToken(
{TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV})) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Descriptor = parseRootDescriptor();
if (!Descriptor.has_value())
return true;
Elements.push_back(*Descriptor);
Element = RootSignatureElement(ElementLoc, *Descriptor);
}

if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Sampler = parseStaticSampler();
if (!Sampler.has_value())
return true;
Elements.push_back(*Sampler);
Element = RootSignatureElement(ElementLoc, *Sampler);
}

if (Element.has_value())
Elements.push_back(*Element);

} while (tryConsumeExpectedToken(TokenKind::pu_comma));

return consumeExpectedToken(TokenKind::end_of_stream,
Expand Down Expand Up @@ -253,10 +263,11 @@ std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() {
do {
if (tryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
TokenKind::kw_UAV, TokenKind::kw_Sampler})) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Clause = parseDescriptorTableClause();
if (!Clause.has_value())
return std::nullopt;
Elements.push_back(*Clause);
Elements.push_back(RootSignatureElement(ElementLoc, *Clause));
Table.NumClauses++;
}

Expand Down
48 changes: 38 additions & 10 deletions clang/lib/Sema/SemaHLSL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1064,21 +1064,25 @@ SemaHLSL::ActOnStartRootSignatureDecl(StringRef Signature) {

void SemaHLSL::ActOnFinishRootSignatureDecl(
SourceLocation Loc, IdentifierInfo *DeclIdent,
SmallVector<llvm::hlsl::rootsig::RootElement> &Elements) {
ArrayRef<hlsl::RootSignatureElement> RootElements) {

if (handleRootSignatureElements(RootElements, Loc))
return;

SmallVector<llvm::hlsl::rootsig::RootElement> Elements;
for (auto &RootSigElement : RootElements)
Elements.push_back(RootSigElement.getElement());

auto *SignatureDecl = HLSLRootSignatureDecl::Create(
SemaRef.getASTContext(), /*DeclContext=*/SemaRef.CurContext, Loc,
DeclIdent, SemaRef.getLangOpts().HLSLRootSigVer, Elements);

if (handleRootSignatureDecl(SignatureDecl, Loc))
return;

SignatureDecl->setImplicit();
SemaRef.PushOnScopeChains(SignatureDecl, SemaRef.getCurScope());
}

bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
SourceLocation Loc) {
bool SemaHLSL::handleRootSignatureElements(
ArrayRef<hlsl::RootSignatureElement> Elements, SourceLocation Loc) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
ArrayRef<hlsl::RootSignatureElement> Elements, SourceLocation Loc) {
ArrayRef<hlsl::RootSignatureElement> Elements) {

We no longer are required to use Loc

// The following conducts analysis on resource ranges to detect and report
// any overlaps in resource ranges.
//
Expand All @@ -1103,9 +1107,15 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
using ResourceRange = llvm::hlsl::rootsig::ResourceRange;
using GroupT = std::pair<ResourceClass, /*Space*/ uint32_t>;

// Introduce a mapping from the collected RangeInfos back to the
// RootSignatureElement that will retain its diagnostics info
llvm::DenseMap<size_t, const hlsl::RootSignatureElement *> InfoIndexMap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
llvm::DenseMap<size_t, const hlsl::RootSignatureElement *> InfoIndexMap;
llvm::SmallDenseMap<size_t, const hlsl::RootSignatureElement *> InfoIndexMap;

size_t InfoIndex = 0;

// 1. Collect RangeInfos
llvm::SmallVector<RangeInfo> Infos;
for (const llvm::hlsl::rootsig::RootElement &Elem : D->getRootElements()) {
for (const hlsl::RootSignatureElement &RootSigElem : Elements) {
const llvm::hlsl::rootsig::RootElement &Elem = RootSigElem.getElement();
if (const auto *Descriptor =
std::get_if<llvm::hlsl::rootsig::RootDescriptor>(&Elem)) {
RangeInfo Info;
Expand All @@ -1116,6 +1126,9 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
llvm::dxil::ResourceClass(llvm::to_underlying(Descriptor->Type));
Info.Space = Descriptor->Space;
Info.Visibility = Descriptor->Visibility;

Info.Index = InfoIndex++;
InfoIndexMap[Info.Index] = &RootSigElem;
Infos.push_back(Info);
} else if (const auto *Constants =
std::get_if<llvm::hlsl::rootsig::RootConstants>(&Elem)) {
Expand All @@ -1126,6 +1139,9 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
Info.Class = llvm::dxil::ResourceClass::CBuffer;
Info.Space = Constants->Space;
Info.Visibility = Constants->Visibility;

Info.Index = InfoIndex++;
InfoIndexMap[Info.Index] = &RootSigElem;
Infos.push_back(Info);
} else if (const auto *Sampler =
std::get_if<llvm::hlsl::rootsig::StaticSampler>(&Elem)) {
Expand All @@ -1136,6 +1152,9 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
Info.Class = llvm::dxil::ResourceClass::Sampler;
Info.Space = Sampler->Space;
Info.Visibility = Sampler->Visibility;

Info.Index = InfoIndex++;
InfoIndexMap[Info.Index] = &RootSigElem;
Infos.push_back(Info);
} else if (const auto *Clause =
std::get_if<llvm::hlsl::rootsig::DescriptorTableClause>(
Expand All @@ -1150,7 +1169,10 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,

Info.Class = Clause->Type;
Info.Space = Clause->Space;

// Note: Clause does not hold the visibility this will need to
Info.Index = InfoIndex++;
InfoIndexMap[Info.Index] = &RootSigElem;
Infos.push_back(Info);
} else if (const auto *Table =
std::get_if<llvm::hlsl::rootsig::DescriptorTable>(&Elem)) {
Expand Down Expand Up @@ -1197,19 +1219,25 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
};

// Helper to report diagnostics
auto ReportOverlap = [this, Loc, &HadOverlap](const RangeInfo *Info,
const RangeInfo *OInfo) {
auto ReportOverlap = [this, InfoIndexMap, &HadOverlap](
const RangeInfo *Info, const RangeInfo *OInfo) {
HadOverlap = true;
auto CommonVis = Info->Visibility == llvm::dxbc::ShaderVisibility::All
? OInfo->Visibility
: Info->Visibility;
this->Diag(Loc, diag::err_hlsl_resource_range_overlap)
const hlsl::RootSignatureElement *Elem = InfoIndexMap.at(Info->Index);
SourceLocation InfoLoc = Elem->getLocation();
this->Diag(InfoLoc, diag::err_hlsl_resource_range_overlap)
<< llvm::to_underlying(Info->Class) << Info->LowerBound
<< /*unbounded=*/(Info->UpperBound == RangeInfo::Unbounded)
<< Info->UpperBound << llvm::to_underlying(OInfo->Class)
<< OInfo->LowerBound
<< /*unbounded=*/(OInfo->UpperBound == RangeInfo::Unbounded)
<< OInfo->UpperBound << Info->Space << CommonVis;

const hlsl::RootSignatureElement *OElem = InfoIndexMap.at(OInfo->Index);
SourceLocation OInfoLoc = OElem->getLocation();
this->Diag(OInfoLoc, diag::note_hlsl_resource_range_here);
};

// 3: Iterate through collected RangeInfos
Expand Down
41 changes: 41 additions & 0 deletions clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl
Original file line number Diff line number Diff line change
@@ -1,49 +1,62 @@
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - %s -verify
// RUN: not %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - %s 2>&1 | FileCheck %s

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges b[42;42] and b[42;42] overlap within space = 0 and visibility = All}}
[RootSignature("CBV(b42), CBV(b42)")]
void bad_root_signature_0() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges t[0;0] and t[0;0] overlap within space = 3 and visibility = All}}
[RootSignature("SRV(t0, space = 3), SRV(t0, space = 3)")]
void bad_root_signature_1() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges u[0;0] and u[0;0] overlap within space = 0 and visibility = Pixel}}
[RootSignature("UAV(u0, visibility = SHADER_VISIBILITY_PIXEL), UAV(u0, visibility = SHADER_VISIBILITY_PIXEL)")]
void bad_root_signature_2() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges u[0;0] and u[0;0] overlap within space = 0 and visibility = Pixel}}
[RootSignature("UAV(u0, visibility = SHADER_VISIBILITY_ALL), UAV(u0, visibility = SHADER_VISIBILITY_PIXEL)")]
void bad_root_signature_3() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges u[0;0] and u[0;0] overlap within space = 0 and visibility = Pixel}}
[RootSignature("UAV(u0, visibility = SHADER_VISIBILITY_PIXEL), UAV(u0, visibility = SHADER_VISIBILITY_ALL)")]
void bad_root_signature_4() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges b[0;0] and b[0;0] overlap within space = 0 and visibility = All}}
[RootSignature("RootConstants(num32BitConstants=4, b0), RootConstants(num32BitConstants=2, b0)")]
void bad_root_signature_5() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges s[3;3] and s[3;3] overlap within space = 0 and visibility = All}}
[RootSignature("StaticSampler(s3), StaticSampler(s3)")]
void bad_root_signature_6() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges t[2;5] and t[0;3] overlap within space = 0 and visibility = All}}
[RootSignature("DescriptorTable(SRV(t0, numDescriptors=4), SRV(t2, numDescriptors=4))")]
void bad_root_signature_7() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges u[2;5] and u[0;unbounded) overlap within space = 0 and visibility = Hull}}
[RootSignature("DescriptorTable(UAV(u0, numDescriptors=unbounded), visibility = SHADER_VISIBILITY_HULL), DescriptorTable(UAV(u2, numDescriptors=4))")]
void bad_root_signature_8() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges b[0;2] and b[2;2] overlap within space = 0 and visibility = All}}
[RootSignature("RootConstants(num32BitConstants=4, b2), DescriptorTable(CBV(b0, numDescriptors=3))")]
void bad_root_signature_9() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges s[4;unbounded) and s[17;17] overlap within space = 0 and visibility = All}}
[RootSignature("StaticSampler(s17), DescriptorTable(Sampler(s0, numDescriptors=3),Sampler(s4, numDescriptors=unbounded))")]
void bad_root_signature_10() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges b[45;45] and b[4;unbounded) overlap within space = 0 and visibility = Geometry}}
[RootSignature("DescriptorTable(CBV(b4, numDescriptors=unbounded)), CBV(b45, visibility = SHADER_VISIBILITY_GEOMETRY)")]
void bad_root_signature_11() {}
Expand All @@ -55,10 +68,38 @@ void bad_root_signature_11() {}
" CBV(b0, numDescriptors = 8), " \
")"

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges b[0;7] and b[1;2] overlap within space = 0 and visibility = All}}
[RootSignature(ReportFirstOverlap)]
void bad_root_signature_12() {}

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges s[2;2] and s[2;2] overlap within space = 0 and visibility = Vertex}}
[RootSignature("StaticSampler(s2, visibility=SHADER_VISIBILITY_ALL), DescriptorTable(Sampler(s2), visibility=SHADER_VISIBILITY_VERTEX)")]
void valid_root_signature_13() {}

#define DemoNoteSourceLocations \
"DescriptorTable( " \
" CBV(b4, numDescriptors = 4), " \
" SRV(t22, numDescriptors = 1), " \
" UAV(u42, numDescriptors = 2), " \
" CBV(b9, numDescriptors = 8), " \
" SRV(t12, numDescriptors = 3), " \
" UAV(u3, numDescriptors = 16), " \
" SRV(t9, numDescriptors = 1), " \
" CBV(b1, numDescriptors = 2), " \
" SRV(t17, numDescriptors = 7), " \
" UAV(u0, numDescriptors = 3), " \
")"

// CHECK: note: expanded from macro 'DemoNoteSourceLocations'
// CHECK-NEXT: [[@LINE-5]] | " SRV(t17, numDescriptors = 7), " \
// CHECK-NEXT: | ^
// CHECK: note: expanded from macro 'DemoNoteSourceLocations'
// CHECK-NEXT: [[@LINE-15]] | " SRV(t22, numDescriptors = 1), "
// CHECK-NEXT: | ^

// expected-note@+2 {{overlapping resource range here}}
// expected-error@+1 {{resource ranges t[17;23] and t[22;22] overlap within space = 0 and visibility = All}}
[RootSignature(DemoNoteSourceLocations)]
void bad_root_signature_14() {}
Loading
Loading