[HLSL][RootSignature] Retain SourceLocation of RootElement for SemaHLSL diagnostics
#147115
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: Finn Plummer (inbelic) ChangesAt the moment, when we report diagnostics from This pr implements a way to retain the source location of a root element when it is parsed, so that we can output the This pr defines a wrapper struct For the reporting of diagnostics, we can now use the retained
Resolves: #145819 Patch is 44.85 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/147115.diff 9 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 968edd967e0c5..4fe0abb972a61 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -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<
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index b0ef617a13c28..9ef5b64d7b4a5 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -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"
@@ -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
@@ -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;
diff --git a/clang/include/clang/Sema/SemaHLSL.h b/clang/include/clang/Sema/SemaHLSL.h
index 7d7eae4db532c..910e0e640796b 100644
--- a/clang/include/clang/Sema/SemaHLSL.h
+++ b/clang/include/clang/Sema/SemaHLSL.h
@@ -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
@@ -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);
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index d3bc6f1e89832..d06bb6a25efa5 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -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()) {
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index ebaf7ba60fa17..86dd01c1a2841 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -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,
@@ -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++;
}
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index ba55f8263e475..f490f957f9667 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -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) {
// The following conducts analysis on resource ranges to detect and report
// any overlaps in resource ranges.
//
@@ -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;
+ 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;
@@ -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)) {
@@ -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)) {
@@ -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>(
@@ -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)) {
@@ -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
diff --git a/clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl b/clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl
index 4b3579d51818a..a021722aea2a4 100644
--- a/clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl
+++ b/clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl
@@ -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() {}
@@ -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() {}
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index ff1697f1bbb9a..f517619df2c2a 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -27,6 +27,7 @@
#include "gtest/gtest.h"
using namespace clang;
+using namespace clang::hlsl;
using namespace llvm::hlsl::rootsig;
namespace {
@@ -135,7 +136,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseEmptyTest) {
TrivialModuleLoader ModLoader;
auto PP = createPP(Source, ModLoader);
- SmallVector<RootElement> Elements;
+ SmallVector<RootSignatureElement> Elements;
hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements,
Signature, *PP);
@@ -171,7 +172,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
TrivialModuleLoader ModLoader;
auto PP = createPP(...
[truncated]
|
Closed
4 tasks
inbelic
force-pushed
the
inbelic/rs-sema-source-loc
branch
from
d66a67d to
b49cfb8
Compare
bogner
reviewed
Jul 8, 2025
Comment on lines
-32
to
+33
| SmallVector<llvm::hlsl::rootsig::RootElement> &Elements, | ||
| SmallVector<RootSignatureElement> &Elements, |
There was a problem hiding this comment.
This is just a change to existing code, but the existing code is a bit concerning. It's never a good idea to pass a (sized) SmallVector as a parameter. In fact, why is the SmallVector a parameter here at all? Wouldn't it make more sense for RootSignatureParser to just own the storage?
There was a problem hiding this comment.
No objection. I will create a follow-up pr to address this.
Comment on lines
36
to
56
| // The index retains its original position before being sorted by group. | ||
| size_t Index; |
There was a problem hiding this comment.
This is only used locally in the diagnostic logic - does it really make sense to include it as a member of the RangeInfo type?
There was a problem hiding this comment.
Okay, so I messed around with this a bit yesterday.
The problem is that when we store a RangeInfo into the underlying IntervalMap, here. We need to either store the associated index with it (as a member, or, as a std::pair<size_t, const RangeInfo *>, or, we will need to let the caller define an external mapping of const RangeInfo * -> size_t.
So I don't think we really solve what we want to by passing it in as a std::pair.
Here is a quick prototype of having an external mapping: https://github.com/inbelic/llvm-project/pull/1/files.
What do you think is the right approach? I think having the index as a member might be the simplest interface.
There was a problem hiding this comment.
The simpler way to do an external mapping would be to use a vector along the lines of
SmallVector<std::pair<RangeInfo, hlsl::RootSignatureElement *>> RangeMap;You'd populate this in the loop in handleRootSignatureElements. Then you'd need to define operator== and operator< on RangeInfo, sort this based on the .first element, and then use std::lower_bound to do a binary search to do the lookup. This uses more memory but avoids the need for details of the algorithm in clang to leak into the RangeInfo type in LLVM. Some care might need to be taken if two RangeInfos can be exactly identical - I'm not sure if that's possible or not.
If we don't want to go that route and it seems more worth it to have storage on the RangeInfo to keep the algorithm as is, we should probably name it something like Cookie instead of Index and label it as generic storage for diagnostic algorithms. This makes it a bit more generic but it still makes for a bit of a code smell in how it breaks the abstraction.
inbelic
changed the base branch from
users/inbelic/pr-147084
to
users/inbelic/pr-147350
inbelic
force-pushed
the
inbelic/rs-sema-source-loc
branch
from
b49cfb8 to
7f19795
Compare
inbelic
commented
Jul 8, 2025
inbelic
changed the base branch from
users/inbelic/pr-147350
to
users/inbelic/pr-147350-rebase
inbelic
force-pushed
the
inbelic/rs-sema-source-loc
branch
from
7f19795 to
c7acf43
Compare
… retain clang diag info
- this struct needs to be accessible to both `Sema` and `Parse` and since `Parse` depends on `Sema` then we need to have it be included from there, so as to not introduce a circular dependency
inbelic
force-pushed
the
inbelic/rs-sema-source-loc
branch
from
a99fced to
1c983b5
Compare
inbelic
force-pushed
the
inbelic/rs-sema-source-loc
branch
from
04f1ebc to
d4eab0d
Compare
joaosaffran
approved these changes
Jul 11, 2025
bogner
approved these changes
Jul 11, 2025
inbelic
force-pushed
the
inbelic/rs-sema-source-loc
branch
from
15ba9bd to
a4018bb
Compare
|
Hi @inbelic ! We're seeing some failures with -DLLVM_ENABLE_EXPENSIVE_CHECKS turned on with this patch. It seems like the diagnostics in SemaHLSL/RootSignature-resource-ranges-err.hlsl end up being printed in a different order from what FileCheck expects. I thought it might be because of the llvm::sort on SemaHLSL.cpp:1159, but changing it to llvm::stable_sort did not help. |
|
Hey @bevin-hansson, thanks for pointing this out. Building locally and taking a look into it now. |
This was referenced Jul 14, 2025
inbelic
added a commit
that referenced
this pull request
Jul 14, 2025
…ent association (#148649) - when there are duplicate (equivalent) `RangeInfo`s created we will attempt to `llvm::sort` or `llvm::stable_sort` them, it does not appear deterministic in which order they will be sorted (because they are equivalent) - when `DLLVM_ENABLE_EXSTENSIVE_CHECKS` is enabled, it appears to deal with this tie-breaker sorting the list differently than when it is not enabled, this causes one of the test cases to fail because the diagnostic is produced, not in a different order, but with a different root element associated with an identical `RangeInfo` - functionally this makes no difference to the diagnostic being produced and the removed test-case was added just as a nicety to demonstrate how the diagnostics might look - the test above the removed will correctly demonstrate that the `SourceLocation` will be set to the correct column, and, the `-verify` portion of this testcase will ensure that we still generate a diagnostic for each duplicate - therefore, it is safe to remove this portion of the test-case that can have a non-deterministic association of root element This resolves the build issues reported [here](#147115 (comment)) and [here](#147800 (comment))
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 14, 2025
…c root element association (#148649) - when there are duplicate (equivalent) `RangeInfo`s created we will attempt to `llvm::sort` or `llvm::stable_sort` them, it does not appear deterministic in which order they will be sorted (because they are equivalent) - when `DLLVM_ENABLE_EXSTENSIVE_CHECKS` is enabled, it appears to deal with this tie-breaker sorting the list differently than when it is not enabled, this causes one of the test cases to fail because the diagnostic is produced, not in a different order, but with a different root element associated with an identical `RangeInfo` - functionally this makes no difference to the diagnostic being produced and the removed test-case was added just as a nicety to demonstrate how the diagnostics might look - the test above the removed will correctly demonstrate that the `SourceLocation` will be set to the correct column, and, the `-verify` portion of this testcase will ensure that we still generate a diagnostic for each duplicate - therefore, it is safe to remove this portion of the test-case that can have a non-deterministic association of root element This resolves the build issues reported [here](llvm/llvm-project#147115 (comment)) and [here](llvm/llvm-project#147800 (comment))
inbelic
added a commit
that referenced
this pull request
Jul 24, 2025
- this is a clean up from a review comment that we should let the parser own the constructed `RootSignatureElement`s Original comment here: #147115 (comment).
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 24, 2025
…#150310) - this is a clean up from a review comment that we should let the parser own the constructed `RootSignatureElement`s Original comment here: llvm/llvm-project#147115 (comment).
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
- this is a clean up from a review comment that we should let the parser own the constructed `RootSignatureElement`s Original comment here: llvm#147115 (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.
At the moment, when we report diagnostics from
SemaHLSLwe 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
SourceLocationof each root element that causes the overlap in the diagnostics during semantic analysis.This pr defines a wrapper struct
clang::hlsl::RootSignatureElementinSemaHLSLthat will contain the underlyingRootElementand can hold any additional diagnostic information. This struct will be what is used inHLSLRootSignatureParserand inSemaHLSL. Then the diagnostic information will be stripped and the underlying element will be stored in theRootSignatureDecl.For the reporting of diagnostics, we can now use the retained
SourceLocationof eachRootElementwhen reporting the range overlap, and we can add anotediagnostic to highlight the other root element as well.RootSignatureElementin thehlslnamespace inSemaHLSL(defined inSemaHLSLbecauseParsehas a dependency onSema)RootSignatureElements and retain the source loction inParseHLSLRootSignatureSemaHLSLwhen it constructs theRootSignatureDeclto take the newRootSignatureElementand store the underlyingRootElementnotediagnostic is produced and that theSourceLocationis seenRootSignatureValidationsapi to ensure the caller sorts and owns the memory of the passed inRangeInfoSourceLocationof both elements being correctly pointed outResolves: #145819