llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-hlsl Author: Finn Plummer (inbelic) <details> <summary>Changes</summary> As was established [previously](https://github.com/llvm/llvm-project/pull/140957), we created a structure to model a resource range and to detect an overlap in a given set of these. However, a resource range only overlaps with another resource range if they have: - equivalent ResourceClass (SRV, UAV, CBuffer, Sampler) - equivalent resource name-space - overlapping shader visibility For instance, the following don't overlap even though they have the same register range: - `CBV(b0)` and `SRV(t0)` (different resource class) - `CBV(b0, space = 0)` and `CBV(b0, space = 1)` (different space) - `CBV(b0, visibility = Pixel)` and `CBV(b0, visibility = Domain)` (non-overlapping visibility) The first two clauses are naturally modelled by a mapping, `map<KeyT, ResourceRange`, where `KeyT` denotes a unique combination of `ResourceClass` and `Space`. However, `Visibility` is not quite as easily mapped (`Visibility = All` would overlap with any other visibility). So we implement `ResourceRanges` as a struct to handle inserting a resource range described by a `RangeInfo` into the appropriate `ResourceRange` and checking for overlaps. This mapping is defined in the structure of `ResourceRanges` and is implemented. Furthermore, we integrate this into `SemaHLSL` to provide a diagnostic for each entry function that uses the invalid root signature. - Implement `ResourceRanges` - Integrate the use of this struct to perform resource range validation in `SemaHLSL` - Add diagnostic testing of error production in `RootSignature-resource-ranges-err.hlsl` - Add testing to ensure no errors are raised in valid root signatures `RootSignature-resource-ranges.hlsl` Part 2 of https://github.com/llvm/llvm-project/issues/129942 --- Full diff: https://github.com/llvm/llvm-project/pull/140962.diff 6 Files Affected: - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5) - (modified) clang/include/clang/Sema/SemaHLSL.h (+2) - (modified) clang/lib/Sema/SemaHLSL.cpp (+98) - (added) clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl (+26) - (added) clang/test/SemaHLSL/RootSignature-resource-ranges.hlsl (+22) - (modified) llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h (+6-2) ``````````diff diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index f0bd5a1174020..b1026e733ec37 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -12967,6 +12967,11 @@ def err_hlsl_expect_arg_const_int_one_or_neg_one: Error< def err_invalid_hlsl_resource_type: Error< "invalid __hlsl_resource_t type attributes">; +def err_hlsl_resource_range_overlap: Error< + "resource ranges %select{t|u|b|s}0[%1;%2] and %select{t|u|b|s}3[%4;%5] " + "overlap within space = %6 and visibility = " + "%select{All|Vertex|Hull|Domain|Geometry|Pixel|Amplification|Mesh}7">; + // Layout randomization diagnostics. def err_non_designated_init_used : Error< "a randomized struct can only be initialized with a designated initializer">; diff --git a/clang/include/clang/Sema/SemaHLSL.h b/clang/include/clang/Sema/SemaHLSL.h index 15182bb27bbdf..a161236d8baa0 100644 --- a/clang/include/clang/Sema/SemaHLSL.h +++ b/clang/include/clang/Sema/SemaHLSL.h @@ -119,6 +119,8 @@ class SemaHLSL : public SemaBase { bool IsCompAssign); void emitLogicalOperatorFixIt(Expr *LHS, Expr *RHS, BinaryOperatorKind Opc); + // Returns true when D is invalid and a diagnostic was produced + bool handleRootSignatureDecl(HLSLRootSignatureDecl *D, 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/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp index e6daa67fcee95..cc53f25c766b0 100644 --- a/clang/lib/Sema/SemaHLSL.cpp +++ b/clang/lib/Sema/SemaHLSL.cpp @@ -39,6 +39,7 @@ #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/ADT/Twine.h" +#include "llvm/Frontend/HLSL/HLSLRootSignature.h" #include "llvm/Support/Casting.h" #include "llvm/Support/DXILABI.h" #include "llvm/Support/ErrorHandling.h" @@ -951,6 +952,101 @@ void SemaHLSL::emitLogicalOperatorFixIt(Expr *LHS, Expr *RHS, << NewFnName << FixItHint::CreateReplacement(FullRange, OS.str()); } +namespace { + +// A resource range overlaps with another resource range if they have: +// - equivalent ResourceClass (SRV, UAV, CBuffer, Sampler) +// - equivalent resource space +// - overlapping visbility +class ResourceRanges { +public: + // KeyT: 32-lsb denotes resource space, and 32-msb denotes resource type enum + using KeyT = uint64_t; + + constexpr static KeyT getKey(const llvm::hlsl::rootsig::RangeInfo &Info) { + uint64_t SpacePacked = (uint64_t)Info.Space; + uint64_t ClassPacked = (uint64_t)llvm::to_underlying(Info.Class); + return (ClassPacked << 32) | SpacePacked; + } + + static const unsigned NumVisEnums = 8; + // (unsigned)llvm::hlsl::rootsig::ShaderVisibility::NumEnums; + +private: + llvm::hlsl::rootsig::ResourceRange::IMap::Allocator Allocator; + + using MapT = llvm::SmallDenseMap<KeyT, llvm::hlsl::rootsig::ResourceRange>; + + MapT RangeMaps[NumVisEnums]; + +public: + // Returns std::nullopt if there was no collision. Otherwise, it will + // return the RangeInfo of the collision + std::optional<const llvm::hlsl::rootsig::RangeInfo *> addRange(const llvm::hlsl::rootsig::RangeInfo &Info) { + MapT &VisRangeMap = RangeMaps[llvm::to_underlying(Info.Vis)]; + auto [It, _] = VisRangeMap.insert({getKey(Info), llvm::hlsl::rootsig::ResourceRange(Allocator)}); + auto Res = It->second.insert(Info); + if (Res.has_value()) + return Res; + + MutableArrayRef<MapT> Maps = + Info.Vis == llvm::hlsl::rootsig::ShaderVisibility::All + ? MutableArrayRef<MapT>{RangeMaps}.drop_front() + : MutableArrayRef<MapT>{RangeMaps}.take_front(); + + for (MapT &CurMap : Maps) { + auto CurIt = CurMap.find(getKey(Info)); + if (CurIt != CurMap.end()) + if (auto Overlapping = CurIt->second.getOverlapping(Info)) + return Overlapping; + } + + return std::nullopt; + } +}; + +} // namespace + +bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D, + SourceLocation Loc) { + auto Elements = D->getRootElements(); + + // First we will go through and collect our range info + llvm::SmallVector<llvm::hlsl::rootsig::RangeInfo> Infos; + for (const auto &Elem : Elements) { + if (const auto *Param = + std::get_if<llvm::hlsl::rootsig::RootParam>(&Elem)) { + llvm::hlsl::rootsig::RangeInfo Info; + Info.LowerBound = Param->Reg.Number; + Info.UpperBound = Info.LowerBound; // use inclusive ranges [] + + Info.Class = Param->Type; + Info.Space = Param->Space; + Info.Vis = Param->Visibility; + Infos.push_back(Info); + } + } + + // Iterate through info and attempt to insert corresponding range + ResourceRanges Ranges; + bool HadOverlap = false; + for (const llvm::hlsl::rootsig::RangeInfo &Info : Infos) + if (auto MaybeOverlappingInfo = Ranges.addRange(Info)) { + const llvm::hlsl::rootsig::RangeInfo *OInfo = MaybeOverlappingInfo.value(); + auto CommonVis = Info.Vis == llvm::hlsl::rootsig::ShaderVisibility::All + ? OInfo->Vis : Info.Vis; + + Diag(Loc, diag::err_hlsl_resource_range_overlap) + << llvm::to_underlying(Info.Class) << Info.LowerBound << Info.UpperBound + << llvm::to_underlying(OInfo->Class) << OInfo->LowerBound << OInfo->UpperBound + << Info.Space << CommonVis; + + HadOverlap = true; + } + + return HadOverlap; +} + void SemaHLSL::handleRootSignatureAttr(Decl *D, const ParsedAttr &AL) { if (AL.getNumArgs() != 1) { Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << AL << 1; @@ -973,6 +1069,8 @@ void SemaHLSL::handleRootSignatureAttr(Decl *D, const ParsedAttr &AL) { if (auto *SignatureDecl = dyn_cast<HLSLRootSignatureDecl>(R.getFoundDecl())) { // Perform validation of constructs here + if (handleRootSignatureDecl(SignatureDecl, AL.getLoc())) + return; D->addAttr(::new (getASTContext()) RootSignatureAttr( getASTContext(), AL, Ident, SignatureDecl)); } diff --git a/clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl b/clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl new file mode 100644 index 0000000000000..a4b5b9c77a248 --- /dev/null +++ b/clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl @@ -0,0 +1,26 @@ +// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - %s -verify + +#define Overlap0 "CBV(b42), CBV(b42)" + +[RootSignature(Overlap0)] // expected-error {{resource ranges b[42;42] and b[42;42] overlap within space = 0 and visibility = All}} +void bad_root_signature_0() {} + +#define Overlap1 "SRV(t0, space = 3), SRV(t0, space = 3)" + +[RootSignature(Overlap1)] // expected-error {{resource ranges t[0;0] and t[0;0] overlap within space = 3 and visibility = All}} +void bad_root_signature_1() {} + +#define Overlap2 "UAV(u0, visibility = SHADER_VISIBILITY_PIXEL), UAV(u0, visibility = SHADER_VISIBILITY_PIXEL)" + +[RootSignature(Overlap2)] // expected-error {{resource ranges u[0;0] and u[0;0] overlap within space = 0 and visibility = Pixel}} +void bad_root_signature_2() {} + +#define Overlap3 "UAV(u0, visibility = SHADER_VISIBILITY_ALL), UAV(u0, visibility = SHADER_VISIBILITY_PIXEL)" + +[RootSignature(Overlap3)] // expected-error {{resource ranges u[0;0] and u[0;0] overlap within space = 0 and visibility = Pixel}} +void bad_root_signature_3() {} + +#define Overlap4 "UAV(u0, visibility = SHADER_VISIBILITY_PIXEL), UAV(u0, visibility = SHADER_VISIBILITY_ALL)" + +[RootSignature(Overlap4)] // expected-error {{resource ranges u[0;0] and u[0;0] overlap within space = 0 and visibility = Pixel}} +void bad_root_signature_4() {} diff --git a/clang/test/SemaHLSL/RootSignature-resource-ranges.hlsl b/clang/test/SemaHLSL/RootSignature-resource-ranges.hlsl new file mode 100644 index 0000000000000..3145b5b332dba --- /dev/null +++ b/clang/test/SemaHLSL/RootSignature-resource-ranges.hlsl @@ -0,0 +1,22 @@ +// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - %s -verify +// expected-no-diagnostics + +#define NoOverlap0 "CBV(b0), CBV(b1)" + +[RootSignature(NoOverlap0)] +void valid_root_signature_0() {} + +#define NoOverlap1 "CBV(b0, visibility = SHADER_VISIBILITY_DOMAIN), CBV(b0, visibility = SHADER_VISIBILITY_PIXEL)" + +[RootSignature(NoOverlap1)] +void valid_root_signature_1() {} + +#define NoOverlap2 "CBV(b0, space = 1), CBV(b0, space = 2)" + +[RootSignature(NoOverlap2)] +void valid_root_signature_2() {} + +#define NoOverlap3 "CBV(b0), SRV(t0)" + +[RootSignature(NoOverlap3)] +void valid_root_signature_3() {} diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h index 3e5054566052b..ec6289e8d858c 100644 --- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h +++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h @@ -199,13 +199,17 @@ class MetadataBuilder { SmallVector<Metadata *> GeneratedMetadata; }; -// RangeInfo holds the information to correctly construct a ResourceRange -// and retains this information to be used for displaying a better diagnostic struct RangeInfo { const static uint32_t Unbounded = static_cast<uint32_t>(-1); + // Interval information uint32_t LowerBound; uint32_t UpperBound; + + // Information retained for diagnostics + llvm::dxil::ResourceClass Class; + uint32_t Space; + ShaderVisibility Vis; }; class ResourceRange { `````````` </details> https://github.com/llvm/llvm-project/pull/140962 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
