Skip to content

Commit 7ecb37b

Browse files
inbelicinbelic
authored
[HLSL][RootSignature] Retain SourceLocation of RootElement for SemaHLSL diagnostics (#147115)
At the moment, when we report diagnostics from `SemaHLSL` we only provide the source location of the root signature attr. This allows for significantly less helpful diagnostics (for eg. reporting resource range overlaps). This pr implements a way to retain the source location of a root element when it is parsed, so that we can output the `SourceLocation` of each root element that causes the overlap in the diagnostics during semantic analysis. This pr defines a wrapper struct `clang::hlsl::RootSignatureElement` in `SemaHLSL` that will contain the underlying `RootElement` and can hold any additional diagnostic information. This struct will be what is used in `HLSLRootSignatureParser` and in `SemaHLSL`. Then the diagnostic information will be stripped and the underlying element will be stored in the `RootSignatureDecl`. For the reporting of diagnostics, we can now use the retained `SourceLocation` of each `RootElement` when reporting the range overlap, and we can add a `note` diagnostic to highlight the other root element as well. - Defines `RootSignatureElement` in the `hlsl` namespace in `SemaHLSL` (defined in `SemaHLSL` because `Parse` has a dependency on `Sema`) - Updates parsing logic to construct `RootSignatureElement`s and retain the source loction in `ParseHLSLRootSignature` - Updates `SemaHLSL` when it constructs the `RootSignatureDecl` to take the new `RootSignatureElement` and store the underlying `RootElement` - Updates the current tests to ensure the new `note` diagnostic is produced and that the `SourceLocation` is seen - Slight update to the `RootSignatureValidations` api to ensure the caller sorts and owns the memory of the passed in `RangeInfo` - Adds a test to demonstrate the `SourceLocation` of both elements being correctly pointed out Resolves: #145819
1 parent a742ee6 commit 7ecb37b

File tree

10 files changed

+311
-143
lines changed

10 files changed

+311
-143
lines changed

‎clang/include/clang/Basic/DiagnosticSemaKinds.td‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13098,6 +13098,7 @@ def err_hlsl_resource_range_overlap: Error<
1309813098
"resource ranges %sub{subst_hlsl_format_ranges}0,1,2,3 and %sub{subst_hlsl_format_ranges}4,5,6,7 "
1309913099
"overlap within space = %8 and visibility = "
1310013100
"%select{All|Vertex|Hull|Domain|Geometry|Pixel|Amplification|Mesh}9">;
13101+
def note_hlsl_resource_range_here: Note<"overlapping resource range here">;
1310113102

1310213103
// Layout randomization diagnostics.
1310313104
def err_non_designated_init_used : Error<

‎clang/include/clang/Parse/ParseHLSLRootSignature.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "clang/Basic/DiagnosticParse.h"
1818
#include "clang/Lex/LexHLSLRootSignature.h"
1919
#include "clang/Lex/Preprocessor.h"
20+
#include "clang/Sema/SemaHLSL.h"
2021

2122
#include "llvm/ADT/SmallVector.h"
2223
#include "llvm/ADT/StringRef.h"
@@ -29,7 +30,7 @@ namespace hlsl {
2930
class RootSignatureParser {
3031
public:
3132
RootSignatureParser(llvm::dxbc::RootSignatureVersion Version,
32-
SmallVector<llvm::hlsl::rootsig::RootElement> &Elements,
33+
SmallVector<RootSignatureElement> &Elements,
3334
StringLiteral *Signature, Preprocessor &PP);
3435

3536
/// Consumes tokens from the Lexer and constructs the in-memory
@@ -201,7 +202,7 @@ class RootSignatureParser {
201202

202203
private:
203204
llvm::dxbc::RootSignatureVersion Version;
204-
SmallVector<llvm::hlsl::rootsig::RootElement> &Elements;
205+
SmallVector<RootSignatureElement> &Elements;
205206
StringLiteral *Signature;
206207
RootSignatureLexer Lexer;
207208
Preprocessor &PP;

‎clang/include/clang/Sema/SemaHLSL.h‎

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,25 @@ class ParsedAttr;
3232
class Scope;
3333
class VarDecl;
3434

35+
namespace hlsl {
36+
37+
// Introduce a wrapper struct around the underlying RootElement. This structure
38+
// will retain extra clang diagnostic information that is not available in llvm.
39+
struct RootSignatureElement {
40+
RootSignatureElement(SourceLocation Loc,
41+
llvm::hlsl::rootsig::RootElement Element)
42+
: Loc(Loc), Element(Element) {}
43+
44+
const llvm::hlsl::rootsig::RootElement &getElement() const { return Element; }
45+
const SourceLocation &getLocation() const { return Loc; }
46+
47+
private:
48+
SourceLocation Loc;
49+
llvm::hlsl::rootsig::RootElement Element;
50+
};
51+
52+
} // namespace hlsl
53+
3554
using llvm::dxil::ResourceClass;
3655

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

131150
/// Creates the Root Signature decl of the parsed Root Signature elements
132151
/// onto the AST and push it onto current Scope
133-
void ActOnFinishRootSignatureDecl(
134-
SourceLocation Loc, IdentifierInfo *DeclIdent,
135-
SmallVector<llvm::hlsl::rootsig::RootElement> &Elements);
136-
137-
// Returns true when D is invalid and a diagnostic was produced
138-
bool handleRootSignatureDecl(HLSLRootSignatureDecl *D, SourceLocation Loc);
152+
void
153+
ActOnFinishRootSignatureDecl(SourceLocation Loc, IdentifierInfo *DeclIdent,
154+
ArrayRef<hlsl::RootSignatureElement> Elements);
155+
156+
// Returns true if any RootSignatureElement is invalid and a diagnostic was
157+
// produced
158+
bool
159+
handleRootSignatureElements(ArrayRef<hlsl::RootSignatureElement> Elements);
139160
void handleRootSignatureAttr(Decl *D, const ParsedAttr &AL);
140161
void handleNumThreadsAttr(Decl *D, const ParsedAttr &AL);
141162
void handleWaveSizeAttr(Decl *D, const ParsedAttr &AL);

‎clang/lib/Parse/ParseDeclCXX.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4948,7 +4948,7 @@ void Parser::ParseHLSLRootSignatureAttributeArgs(ParsedAttributes &Attrs) {
49484948
// signature string and construct the in-memory elements
49494949
if (!Found) {
49504950
// Invoke the root signature parser to construct the in-memory constructs
4951-
SmallVector<llvm::hlsl::rootsig::RootElement> RootElements;
4951+
SmallVector<hlsl::RootSignatureElement> RootElements;
49524952
hlsl::RootSignatureParser Parser(getLangOpts().HLSLRootSigVer, RootElements,
49534953
Signature, PP);
49544954
if (Parser.parse()) {

‎clang/lib/Parse/ParseHLSLRootSignature.cpp‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ using TokenKind = RootSignatureToken::Kind;
1919

2020
RootSignatureParser::RootSignatureParser(
2121
llvm::dxbc::RootSignatureVersion Version,
22-
SmallVector<RootElement> &Elements, StringLiteral *Signature,
22+
SmallVector<RootSignatureElement> &Elements, StringLiteral *Signature,
2323
Preprocessor &PP)
2424
: Version(Version), Elements(Elements), Signature(Signature),
2525
Lexer(Signature->getString()), PP(PP), CurToken(0) {}
@@ -29,31 +29,36 @@ bool RootSignatureParser::parse() {
2929
// end of the stream
3030
while (!peekExpectedToken(TokenKind::end_of_stream)) {
3131
if (tryConsumeExpectedToken(TokenKind::kw_RootFlags)) {
32+
SourceLocation ElementLoc = getTokenLocation(CurToken);
3233
auto Flags = parseRootFlags();
3334
if (!Flags.has_value())
3435
return true;
35-
Elements.push_back(*Flags);
36+
Elements.emplace_back(ElementLoc, *Flags);
3637
} else if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) {
38+
SourceLocation ElementLoc = getTokenLocation(CurToken);
3739
auto Constants = parseRootConstants();
3840
if (!Constants.has_value())
3941
return true;
40-
Elements.push_back(*Constants);
42+
Elements.emplace_back(ElementLoc, *Constants);
4143
} else if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
44+
SourceLocation ElementLoc = getTokenLocation(CurToken);
4245
auto Table = parseDescriptorTable();
4346
if (!Table.has_value())
4447
return true;
45-
Elements.push_back(*Table);
48+
Elements.emplace_back(ElementLoc, *Table);
4649
} else if (tryConsumeExpectedToken(
4750
{TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV})) {
51+
SourceLocation ElementLoc = getTokenLocation(CurToken);
4852
auto Descriptor = parseRootDescriptor();
4953
if (!Descriptor.has_value())
5054
return true;
51-
Elements.push_back(*Descriptor);
55+
Elements.emplace_back(ElementLoc, *Descriptor);
5256
} else if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) {
57+
SourceLocation ElementLoc = getTokenLocation(CurToken);
5358
auto Sampler = parseStaticSampler();
5459
if (!Sampler.has_value())
5560
return true;
56-
Elements.push_back(*Sampler);
61+
Elements.emplace_back(ElementLoc, *Sampler);
5762
}
5863

5964
// ',' denotes another element, otherwise, expected to be at end of stream
@@ -245,10 +250,11 @@ std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() {
245250
if (tryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
246251
TokenKind::kw_UAV, TokenKind::kw_Sampler})) {
247252
// DescriptorTableClause - CBV, SRV, UAV, or Sampler
253+
SourceLocation ElementLoc = getTokenLocation(CurToken);
248254
auto Clause = parseDescriptorTableClause();
249255
if (!Clause.has_value())
250256
return std::nullopt;
251-
Elements.push_back(*Clause);
257+
Elements.emplace_back(ElementLoc, *Clause);
252258
Table.NumClauses++;
253259
} else if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
254260
// visibility = SHADER_VISIBILITY

‎clang/lib/Sema/SemaHLSL.cpp‎

Lines changed: 73 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1064,27 +1064,33 @@ SemaHLSL::ActOnStartRootSignatureDecl(StringRef Signature) {
10641064

10651065
void SemaHLSL::ActOnFinishRootSignatureDecl(
10661066
SourceLocation Loc, IdentifierInfo *DeclIdent,
1067-
SmallVector<llvm::hlsl::rootsig::RootElement> &Elements) {
1067+
ArrayRef<hlsl::RootSignatureElement> RootElements) {
1068+
1069+
if (handleRootSignatureElements(RootElements))
1070+
return;
1071+
1072+
SmallVector<llvm::hlsl::rootsig::RootElement> Elements;
1073+
for (auto &RootSigElement : RootElements)
1074+
Elements.push_back(RootSigElement.getElement());
10681075

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

1073-
if (handleRootSignatureDecl(SignatureDecl, Loc))
1074-
return;
1075-
10761080
SignatureDecl->setImplicit();
10771081
SemaRef.PushOnScopeChains(SignatureDecl, SemaRef.getCurScope());
10781082
}
10791083

1080-
bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
1081-
SourceLocation Loc) {
1084+
bool SemaHLSL::handleRootSignatureElements(
1085+
ArrayRef<hlsl::RootSignatureElement> Elements) {
10821086
using RangeInfo = llvm::hlsl::rootsig::RangeInfo;
10831087
using OverlappingRanges = llvm::hlsl::rootsig::OverlappingRanges;
1088+
using InfoPairT = std::pair<RangeInfo, const hlsl::RootSignatureElement *>;
10841089

10851090
// 1. Collect RangeInfos
1086-
llvm::SmallVector<RangeInfo> Infos;
1087-
for (const llvm::hlsl::rootsig::RootElement &Elem : D->getRootElements()) {
1091+
llvm::SmallVector<InfoPairT> InfoPairs;
1092+
for (const hlsl::RootSignatureElement &RootSigElem : Elements) {
1093+
const llvm::hlsl::rootsig::RootElement &Elem = RootSigElem.getElement();
10881094
if (const auto *Descriptor =
10891095
std::get_if<llvm::hlsl::rootsig::RootDescriptor>(&Elem)) {
10901096
RangeInfo Info;
@@ -1095,7 +1101,8 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
10951101
llvm::dxil::ResourceClass(llvm::to_underlying(Descriptor->Type));
10961102
Info.Space = Descriptor->Space;
10971103
Info.Visibility = Descriptor->Visibility;
1098-
Infos.push_back(Info);
1104+
1105+
InfoPairs.push_back({Info, &RootSigElem});
10991106
} else if (const auto *Constants =
11001107
std::get_if<llvm::hlsl::rootsig::RootConstants>(&Elem)) {
11011108
RangeInfo Info;
@@ -1105,7 +1112,8 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
11051112
Info.Class = llvm::dxil::ResourceClass::CBuffer;
11061113
Info.Space = Constants->Space;
11071114
Info.Visibility = Constants->Visibility;
1108-
Infos.push_back(Info);
1115+
1116+
InfoPairs.push_back({Info, &RootSigElem});
11091117
} else if (const auto *Sampler =
11101118
std::get_if<llvm::hlsl::rootsig::StaticSampler>(&Elem)) {
11111119
RangeInfo Info;
@@ -1115,7 +1123,8 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
11151123
Info.Class = llvm::dxil::ResourceClass::Sampler;
11161124
Info.Space = Sampler->Space;
11171125
Info.Visibility = Sampler->Visibility;
1118-
Infos.push_back(Info);
1126+
1127+
InfoPairs.push_back({Info, &RootSigElem});
11191128
} else if (const auto *Clause =
11201129
std::get_if<llvm::hlsl::rootsig::DescriptorTableClause>(
11211130
&Elem)) {
@@ -1129,38 +1138,83 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
11291138

11301139
Info.Class = Clause->Type;
11311140
Info.Space = Clause->Space;
1141+
11321142
// Note: Clause does not hold the visibility this will need to
1133-
Infos.push_back(Info);
1143+
InfoPairs.push_back({Info, &RootSigElem});
11341144
} else if (const auto *Table =
11351145
std::get_if<llvm::hlsl::rootsig::DescriptorTable>(&Elem)) {
11361146
// Table holds the Visibility of all owned Clauses in Table, so iterate
11371147
// owned Clauses and update their corresponding RangeInfo
1138-
assert(Table->NumClauses <= Infos.size() && "RootElement");
1148+
assert(Table->NumClauses <= InfoPairs.size() && "RootElement");
11391149
// The last Table->NumClauses elements of Infos are the owned Clauses
11401150
// generated RangeInfo
11411151
auto TableInfos =
1142-
MutableArrayRef<RangeInfo>(Infos).take_back(Table->NumClauses);
1143-
for (RangeInfo &Info : TableInfos)
1144-
Info.Visibility = Table->Visibility;
1152+
MutableArrayRef<InfoPairT>(InfoPairs).take_back(Table->NumClauses);
1153+
for (InfoPairT &Pair : TableInfos)
1154+
Pair.first.Visibility = Table->Visibility;
11451155
}
11461156
}
11471157

1148-
// Helper to report diagnostics
1149-
auto ReportOverlap = [this, Loc](OverlappingRanges Overlap) {
1158+
// 2. Sort with the RangeInfo <operator to prepare it for findOverlapping
1159+
llvm::sort(InfoPairs,
1160+
[](InfoPairT A, InfoPairT B) { return A.first < B.first; });
1161+
1162+
llvm::SmallVector<RangeInfo> Infos;
1163+
for (const InfoPairT &Pair : InfoPairs)
1164+
Infos.push_back(Pair.first);
1165+
1166+
// Helpers to report diagnostics
1167+
uint32_t DuplicateCounter = 0;
1168+
using ElemPair = std::pair<const hlsl::RootSignatureElement *,
1169+
const hlsl::RootSignatureElement *>;
1170+
auto GetElemPair = [&Infos, &InfoPairs, &DuplicateCounter](
1171+
OverlappingRanges Overlap) -> ElemPair {
1172+
// Given we sorted the InfoPairs (and by implication) Infos, and,
1173+
// that Overlap.B is the item retrieved from the ResourceRange. Then it is
1174+
// guarenteed that Overlap.B <= Overlap.A.
1175+
//
1176+
// So we will find Overlap.B first and then continue to find Overlap.A
1177+
// after
1178+
auto InfoB = std::lower_bound(Infos.begin(), Infos.end(), *Overlap.B);
1179+
auto DistB = std::distance(Infos.begin(), InfoB);
1180+
auto PairB = InfoPairs.begin();
1181+
std::advance(PairB, DistB);
1182+
1183+
auto InfoA = std::lower_bound(InfoB, Infos.end(), *Overlap.A);
1184+
// Similarily, from the property that we have sorted the RangeInfos,
1185+
// all duplicates will be processed one after the other. So
1186+
// DuplicateCounter can be re-used for each set of duplicates we
1187+
// encounter as we handle incoming errors
1188+
DuplicateCounter = InfoA == InfoB ? DuplicateCounter + 1 : 0;
1189+
auto DistA = std::distance(InfoB, InfoA) + DuplicateCounter;
1190+
auto PairA = PairB;
1191+
std::advance(PairA, DistA);
1192+
1193+
return {PairA->second, PairB->second};
1194+
};
1195+
1196+
auto ReportOverlap = [this, &GetElemPair](OverlappingRanges Overlap) {
1197+
auto Pair = GetElemPair(Overlap);
11501198
const RangeInfo *Info = Overlap.A;
1199+
const hlsl::RootSignatureElement *Elem = Pair.first;
11511200
const RangeInfo *OInfo = Overlap.B;
1201+
11521202
auto CommonVis = Info->Visibility == llvm::dxbc::ShaderVisibility::All
11531203
? OInfo->Visibility
11541204
: Info->Visibility;
1155-
this->Diag(Loc, diag::err_hlsl_resource_range_overlap)
1205+
this->Diag(Elem->getLocation(), diag::err_hlsl_resource_range_overlap)
11561206
<< llvm::to_underlying(Info->Class) << Info->LowerBound
11571207
<< /*unbounded=*/(Info->UpperBound == RangeInfo::Unbounded)
11581208
<< Info->UpperBound << llvm::to_underlying(OInfo->Class)
11591209
<< OInfo->LowerBound
11601210
<< /*unbounded=*/(OInfo->UpperBound == RangeInfo::Unbounded)
11611211
<< OInfo->UpperBound << Info->Space << CommonVis;
1212+
1213+
const hlsl::RootSignatureElement *OElem = Pair.second;
1214+
this->Diag(OElem->getLocation(), diag::note_hlsl_resource_range_here);
11621215
};
11631216

1217+
// 3. Invoke find overlapping ranges
11641218
llvm::SmallVector<OverlappingRanges> Overlaps =
11651219
llvm::hlsl::rootsig::findOverlappingRanges(Infos);
11661220
for (OverlappingRanges Overlap : Overlaps)

0 commit comments

Comments
 (0)