llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Damyan Pepper (damyanp) <details> <summary>Changes</summary> Remove: * DescriptorType enum - this almost exactly shadowed the ResourceClass enum * ClauseType aliased ResourceClass Although these were introduced to make the HLSL root signature handling code a bit cleaner, they were ultimately causing confusion as they appeared to be unique enums that needed to be converted between. Closes #<!-- -->153890 --- Full diff: https://github.com/llvm/llvm-project/pull/153909.diff 5 Files Affected: - (modified) clang/lib/Parse/ParseHLSLRootSignature.cpp (+7-7) - (modified) clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp (+23-23) - (modified) llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h (+13-12) - (modified) llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp (+4-5) - (modified) llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp (+2-5) ``````````diff diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 98dc458f7adc5..5490c61f52356 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -234,15 +234,15 @@ std::optional<RootDescriptor> RootSignatureParser::parseRootDescriptor() { default: llvm_unreachable("Switch for consumed token was not provided"); case TokenKind::kw_CBV: - Descriptor.Type = DescriptorType::CBuffer; + Descriptor.Type = ResourceClass::CBuffer; ExpectedReg = TokenKind::bReg; break; case TokenKind::kw_SRV: - Descriptor.Type = DescriptorType::SRV; + Descriptor.Type = ResourceClass::SRV; ExpectedReg = TokenKind::tReg; break; case TokenKind::kw_UAV: - Descriptor.Type = DescriptorType::UAV; + Descriptor.Type = ResourceClass::UAV; ExpectedReg = TokenKind::uReg; break; } @@ -360,19 +360,19 @@ RootSignatureParser::parseDescriptorTableClause() { default: llvm_unreachable("Switch for consumed token was not provided"); case TokenKind::kw_CBV: - Clause.Type = ClauseType::CBuffer; + Clause.Type = ResourceClass::CBuffer; ExpectedReg = TokenKind::bReg; break; case TokenKind::kw_SRV: - Clause.Type = ClauseType::SRV; + Clause.Type = ResourceClass::SRV; ExpectedReg = TokenKind::tReg; break; case TokenKind::kw_UAV: - Clause.Type = ClauseType::UAV; + Clause.Type = ResourceClass::UAV; ExpectedReg = TokenKind::uReg; break; case TokenKind::kw_Sampler: - Clause.Type = ClauseType::Sampler; + Clause.Type = ResourceClass::Sampler; ExpectedReg = TokenKind::sReg; break; } diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 44f6b0469f38e..44c0978a243bc 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -180,7 +180,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) { // First Descriptor Table with 4 elements RootElement Elem = Elements[0].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::CBuffer); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::CBuffer); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Reg.ViewType, RegisterType::BReg); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Reg.Number, 0u); @@ -193,7 +193,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) { Elem = Elements[1].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::SRV); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::SRV); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Reg.ViewType, RegisterType::TReg); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Reg.Number, 42u); @@ -205,7 +205,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) { Elem = Elements[2].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::Sampler); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::Sampler); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Reg.ViewType, RegisterType::SReg); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Reg.Number, 987u); @@ -218,7 +218,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) { Elem = Elements[3].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::UAV); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::UAV); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Reg.ViewType, RegisterType::UReg); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Reg.Number, 4294967294u); @@ -445,7 +445,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidSamplerFlagsTest) { auto Elements = Parser.getElements(); RootElement Elem = Elements[0].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::Sampler); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::Sampler); auto ValidSamplerFlags = llvm::dxbc::DescriptorRangeFlags::DescriptorsVolatile; ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags, ValidSamplerFlags); @@ -591,7 +591,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootDescriptorsTest) { RootElement Elem = Elements[0].getElement(); ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem)); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::CBuffer); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::CBuffer); ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.ViewType, RegisterType::BReg); ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.Number, 0u); ASSERT_EQ(std::get<RootDescriptor>(Elem).Space, 0u); @@ -602,7 +602,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootDescriptorsTest) { Elem = Elements[1].getElement(); ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem)); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::SRV); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::SRV); ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.ViewType, RegisterType::TReg); ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.Number, 42u); ASSERT_EQ(std::get<RootDescriptor>(Elem).Space, 4u); @@ -616,7 +616,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootDescriptorsTest) { Elem = Elements[2].getElement(); ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem)); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::UAV); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::UAV); ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.ViewType, RegisterType::UReg); ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.Number, 34893247u); ASSERT_EQ(std::get<RootDescriptor>(Elem).Space, 0u); @@ -628,7 +628,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootDescriptorsTest) { RootDescriptorFlags::DataVolatile); Elem = Elements[3].getElement(); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::CBuffer); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::CBuffer); ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.ViewType, RegisterType::BReg); ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.Number, 0u); ASSERT_EQ(std::get<RootDescriptor>(Elem).Space, 0u); @@ -696,17 +696,17 @@ TEST_F(ParseHLSLRootSignatureTest, ValidVersion10Test) { auto DefRootDescriptorFlag = llvm::dxbc::RootDescriptorFlags::DataVolatile; RootElement Elem = Elements[0].getElement(); ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem)); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::CBuffer); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::CBuffer); ASSERT_EQ(std::get<RootDescriptor>(Elem).Flags, DefRootDescriptorFlag); Elem = Elements[1].getElement(); ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem)); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::SRV); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::SRV); ASSERT_EQ(std::get<RootDescriptor>(Elem).Flags, DefRootDescriptorFlag); Elem = Elements[2].getElement(); ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem)); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::UAV); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::UAV); ASSERT_EQ(std::get<RootDescriptor>(Elem).Flags, DefRootDescriptorFlag); auto ValidNonSamplerFlags = @@ -714,22 +714,22 @@ TEST_F(ParseHLSLRootSignatureTest, ValidVersion10Test) { llvm::dxbc::DescriptorRangeFlags::DataVolatile; Elem = Elements[3].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::CBuffer); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::CBuffer); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags, ValidNonSamplerFlags); Elem = Elements[4].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::SRV); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::SRV); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags, ValidNonSamplerFlags); Elem = Elements[5].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::UAV); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::UAV); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags, ValidNonSamplerFlags); Elem = Elements[6].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::Sampler); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::Sampler); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags, llvm::dxbc::DescriptorRangeFlags::DescriptorsVolatile); @@ -767,43 +767,43 @@ TEST_F(ParseHLSLRootSignatureTest, ValidVersion11Test) { auto Elements = Parser.getElements(); RootElement Elem = Elements[0].getElement(); ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem)); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::CBuffer); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::CBuffer); ASSERT_EQ(std::get<RootDescriptor>(Elem).Flags, llvm::dxbc::RootDescriptorFlags::DataStaticWhileSetAtExecute); Elem = Elements[1].getElement(); ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem)); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::SRV); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::SRV); ASSERT_EQ(std::get<RootDescriptor>(Elem).Flags, llvm::dxbc::RootDescriptorFlags::DataStaticWhileSetAtExecute); Elem = Elements[2].getElement(); ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem)); - ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::UAV); + ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, ResourceClass::UAV); ASSERT_EQ(std::get<RootDescriptor>(Elem).Flags, llvm::dxbc::RootDescriptorFlags::DataVolatile); Elem = Elements[3].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::CBuffer); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::CBuffer); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags, llvm::dxbc::DescriptorRangeFlags::DataStaticWhileSetAtExecute); Elem = Elements[4].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::SRV); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::SRV); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags, llvm::dxbc::DescriptorRangeFlags::DataStaticWhileSetAtExecute); Elem = Elements[5].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::UAV); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::UAV); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags, llvm::dxbc::DescriptorRangeFlags::DataVolatile); Elem = Elements[6].getElement(); ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem)); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::Sampler); + ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ResourceClass::Sampler); ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags, llvm::dxbc::DescriptorRangeFlags::None); diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h index e44612af071bc..87777fddc9157 100644 --- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h +++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h @@ -42,10 +42,9 @@ struct RootConstants { dxbc::ShaderVisibility Visibility = dxbc::ShaderVisibility::All; }; -enum class DescriptorType : uint8_t { SRV = 0, UAV, CBuffer }; // Models RootDescriptor : CBV | SRV | UAV, by collecting like parameters struct RootDescriptor { - DescriptorType Type; + dxil::ResourceClass Type; Register Reg; uint32_t Space = 0; dxbc::ShaderVisibility Visibility = dxbc::ShaderVisibility::All; @@ -60,13 +59,16 @@ struct RootDescriptor { assert(Version == llvm::dxbc::RootSignatureVersion::V1_1 && "Specified an invalid root signature version"); switch (Type) { - case DescriptorType::CBuffer: - case DescriptorType::SRV: + case dxil::ResourceClass::CBuffer: + case dxil::ResourceClass::SRV: Flags = dxbc::RootDescriptorFlags::DataStaticWhileSetAtExecute; break; - case DescriptorType::UAV: + case dxil::ResourceClass::UAV: Flags = dxbc::RootDescriptorFlags::DataVolatile; break; + case dxil::ResourceClass::Sampler: + llvm_unreachable( + "ResourceClass::Sampler is not valid for RootDescriptors"); } } }; @@ -82,9 +84,8 @@ struct DescriptorTable { static const uint32_t NumDescriptorsUnbounded = 0xffffffff; static const uint32_t DescriptorTableOffsetAppend = 0xffffffff; // Models DTClause : CBV | SRV | UAV | Sampler, by collecting like parameters -using ClauseType = llvm::dxil::ResourceClass; struct DescriptorTableClause { - ClauseType Type; + dxil::ResourceClass Type; Register Reg; uint32_t NumDescriptors = 1; uint32_t Space = 0; @@ -94,7 +95,7 @@ struct DescriptorTableClause { void setDefaultFlags(dxbc::RootSignatureVersion Version) { if (Version == dxbc::RootSignatureVersion::V1_0) { Flags = dxbc::DescriptorRangeFlags::DescriptorsVolatile; - if (Type != ClauseType::Sampler) + if (Type != dxil::ResourceClass::Sampler) Flags |= dxbc::DescriptorRangeFlags::DataVolatile; return; } @@ -102,14 +103,14 @@ struct DescriptorTableClause { assert(Version == dxbc::RootSignatureVersion::V1_1 && "Specified an invalid root signature version"); switch (Type) { - case ClauseType::CBuffer: - case ClauseType::SRV: + case dxil::ResourceClass::CBuffer: + case dxil::ResourceClass::SRV: Flags = dxbc::DescriptorRangeFlags::DataStaticWhileSetAtExecute; break; - case ClauseType::UAV: + case dxil::ResourceClass::UAV: Flags = dxbc::DescriptorRangeFlags::DataVolatile; break; - case ClauseType::Sampler: + case dxil::ResourceClass::Sampler: Flags = dxbc::DescriptorRangeFlags::None; break; } diff --git a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp index 050cc46e8c9b0..14c729b1ad97b 100644 --- a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp +++ b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp @@ -92,9 +92,9 @@ static raw_ostream &operator<<(raw_ostream &OS, return OS; } -static raw_ostream &operator<<(raw_ostream &OS, const ClauseType &Type) { - OS << enumToStringRef(dxil::ResourceClass(llvm::to_underlying(Type)), - dxil::getResourceClasses()); +static raw_ostream &operator<<(raw_ostream &OS, + const dxil::ResourceClass &Type) { + OS << enumToStringRef(Type, dxil::getResourceClasses()); return OS; } @@ -153,8 +153,7 @@ raw_ostream &operator<<(raw_ostream &OS, const DescriptorTableClause &Clause) { } raw_ostream &operator<<(raw_ostream &OS, const RootDescriptor &Descriptor) { - ClauseType Type = ClauseType(llvm::to_underlying(Descriptor.Type)); - OS << "Root" << Type << "(" << Descriptor.Reg + OS << "Root" << Descriptor.Type << "(" << Descriptor.Reg << ", space = " << Descriptor.Space << ", visibility = " << Descriptor.Visibility << ", flags = " << Descriptor.Flags << ")"; diff --git a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp index 157bfc665b207..ae58d80e12d5f 100644 --- a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp +++ b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp @@ -120,8 +120,7 @@ MDNode *MetadataBuilder::BuildRootConstants(const RootConstants &Constants) { MDNode *MetadataBuilder::BuildRootDescriptor(const RootDescriptor &Descriptor) { IRBuilder<> Builder(Ctx); StringRef ResName = - enumToStringRef(dxil::ResourceClass(to_underlying(Descriptor.Type)), - dxil::getResourceClasses()); + enumToStringRef(Descriptor.Type, dxil::getResourceClasses()); assert(!ResName.empty() && "Provided an invalid Resource Class"); SmallString<7> Name({"Root", ResName}); Metadata *Operands[] = { @@ -161,9 +160,7 @@ MDNode *MetadataBuilder::BuildDescriptorTable(const DescriptorTable &Table) { MDNode *MetadataBuilder::BuildDescriptorTableClause( const DescriptorTableClause &Clause) { IRBuilder<> Builder(Ctx); - StringRef ResName = - enumToStringRef(dxil::ResourceClass(to_underlying(Clause.Type)), - dxil::getResourceClasses()); + StringRef ResName = enumToStringRef(Clause.Type, dxil::getResourceClasses()); assert(!ResName.empty() && "Provided an invalid Resource Class"); Metadata *Operands[] = { MDString::get(Ctx, ResName), `````````` </details> https://github.com/llvm/llvm-project/pull/153909 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
