[NFC] Refactoring MCDXBC to support out of order storage of root parameters #137284
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,18 +9,81 @@ | |
| #include "llvm/BinaryFormat/DXContainer.h" | ||
| #include <cstdint> | ||
| #include <limits> | ||
| #include <variant> | ||
|
|
||
| namespace llvm { | ||
|
|
||
| class raw_ostream; | ||
| namespace mcdxbc { | ||
|
|
||
| struct RootParameterInfo { | ||
| dxbc::RootParameterHeader Header; | ||
| size_t Location; | ||
|
|
||
| RootParameterInfo() = default; | ||
|
|
||
| RootParameterInfo(dxbc::RootParameterHeader H, size_t L) | ||
| : Header(H), Location(L) {} | ||
| }; | ||
|
|
||
| using RootDescriptor = std::variant<dxbc::RTS0::v1::RootDescriptor, | ||
| dxbc::RTS0::v2::RootDescriptor>; | ||
| using ParametersView = std::variant<const dxbc::RootConstants *, | ||
| const dxbc::RTS0::v1::RootDescriptor *, | ||
| const dxbc::RTS0::v2::RootDescriptor *>; | ||
| struct RootParametersContainer { | ||
| SmallVector<RootParameterInfo> ParametersInfo; | ||
|
|
||
| SmallVector<dxbc::RootConstants> Constants; | ||
| SmallVector<RootDescriptor> Descriptors; | ||
|
There was a problem hiding this comment. We're not tied to the binary format itself here, so I think it would be quite a bit simpler to just have this vector contain the SmallVector<dxbc::RTS0::v2::RootDescriptor> Descriptors;This should end up simplifying support::endian::write(BOS, Descriptor.ShaderRegister,
llvm::endianness::little);
support::endian::write(BOS, Descriptor.RegisterSpace,
llvm::endianness::little);
if (Version > 1)
support::endian::write(BOS, Descriptor.Flags, llvm::endianness::little); |
||
|
|
||
| void addInfo(dxbc::RootParameterHeader H, size_t L) { | ||
| ParametersInfo.push_back(RootParameterInfo(H, L)); | ||
| } | ||
|
|
||
| void addParameter(dxbc::RootParameterHeader H, dxbc::RootConstants C) { | ||
| addInfo(H, Constants.size()); | ||
| Constants.push_back(C); | ||
| } | ||
|
|
||
| void addParameter(dxbc::RootParameterHeader H, | ||
| dxbc::RTS0::v1::RootDescriptor D) { | ||
| addInfo(H, Descriptors.size()); | ||
| Descriptors.push_back(D); | ||
| } | ||
|
|
||
| void addParameter(dxbc::RootParameterHeader H, | ||
| dxbc::RTS0::v2::RootDescriptor D) { | ||
| addInfo(H, Descriptors.size()); | ||
| Descriptors.push_back(D); | ||
| } | ||
|
|
||
| std::optional<ParametersView> getParameter(const RootParameterInfo *H) const { | ||
| switch (H->Header.ParameterType) { | ||
|
There was a problem hiding this comment. Is this switch exhaustive? If it is then do we need to make the return optional? There was a problem hiding this comment. Yes, is using an optional necessary here? Is the nullopt case reachable? If it should not be reachable likely best to use an llvm_unreachable instead. There was a problem hiding this comment. Optional is necessary here, because yaml2obj needs to be able to write invalid root signatures representations, since we used it as a testing tool. There was a problem hiding this comment. I don't understand, can you elaborate. What does writing an invalid root signature look like and how does it interact with this code? There was a problem hiding this comment. Sure. Some context, Root Parameters are split into a Header and a Data section. The Header contains a RootParameter type field, specifying which kind of data is being stored: Root Constants, Root Descriptors or Descriptor Tables. The header also contains an offset, pointing to the exact location of the data in the binary file. Here is an test example showing what an invalid root signature look like: https://github.com/llvm/llvm-project/blob/038d357dde4907d39f6a3fabbaf48dc39cf9dc60/llvm/test/ObjectYAML/DXContainer/RootSignature-InvalidType.yaml. Notice that in such test there is no data section, only the header section. Since I don't know what kind of data it is/the data is not supported, it is not possible to write it. |
||
| case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): | ||
| return &Constants[H->Location]; | ||
| case llvm::to_underlying(dxbc::RootParameterType::CBV): | ||
| case llvm::to_underlying(dxbc::RootParameterType::SRV): | ||
| case llvm::to_underlying(dxbc::RootParameterType::UAV): | ||
| const RootDescriptor &VersionedParam = Descriptors[H->Location]; | ||
| if (std::holds_alternative<dxbc::RTS0::v1::RootDescriptor>( | ||
| VersionedParam)) { | ||
| return &std::get<dxbc::RTS0::v1::RootDescriptor>(VersionedParam); | ||
| } | ||
| return &std::get<dxbc::RTS0::v2::RootDescriptor>(VersionedParam); | ||
| } | ||
|
|
||
| return std::nullopt; | ||
| } | ||
|
There was a problem hiding this comment. I'm not really convinced that the abstraction that the const std::pair<uint32_t, uint32_t>
getTypeAndLocForParameter(uint32_t Index) const {
const RootParameterInfo &Info = ParametersInfo[Index];
return {Info.Header.ParameterType, Info.Location};
}
const dxbc::RootConstants &getConstant(size_t Index) const {
return Constants[Index];
}
const dxbc::RTS0::v2::RootDescriptor &getRootDescriptor(size_t Index) const {
return Descriptors[Index];
}The logic to use this is more or less the same - instead of the - auto P = ParametersContainer.getParameter(ParametersContainer[I]);
- if (std::holds_alternative<const dxbc::RootConstants *>(P.value())) {
- auto *Constants = std::get<const dxbc::RootConstants *>(P.value());
- support::endian::write(BOS, Constants->ShaderRegister,
- llvm::endianness::little);
+ const auto &[Type, Loc] = ParametersContainer.getTypeAndLocForParameter(I);
+ switch (Type) {
+ case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+ const dxbc::RootConstants &Constants =
+ ParametersContainer.getConstant(Loc);
+ support::endian::write(BOS, Constants.ShaderRegister,
+ llvm::endianness::little);I think |
||
|
|
||
| size_t size() const { return ParametersInfo.size(); } | ||
|
|
||
| SmallVector<RootParameterInfo>::const_iterator begin() const { | ||
| return ParametersInfo.begin(); | ||
| } | ||
| SmallVector<RootParameterInfo>::const_iterator end() const { | ||
| return ParametersInfo.end(); | ||
| } | ||
| }; | ||
| struct RootSignatureDesc { | ||
|
|
||
|
|
@@ -29,7 +92,7 @@ struct RootSignatureDesc { | |
| uint32_t RootParameterOffset = 0U; | ||
| uint32_t StaticSamplersOffset = 0u; | ||
| uint32_t NumStaticSamplers = 0u; | ||
| mcdxbc::RootParametersContainer ParametersContainer; | ||
|
|
||
| void write(raw_ostream &OS) const; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,31 +75,32 @@ static bool parseRootConstants(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD, | |
| if (RootConstantNode->getNumOperands() != 5) | ||
| return reportError(Ctx, "Invalid format for RootConstants Element"); | ||
|
|
||
| dxbc::RootParameterHeader Header; | ||
| Header.ParameterType = | ||
| llvm::to_underlying(dxbc::RootParameterType::Constants32Bit); | ||
|
|
||
| if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 1)) | ||
| Header.ShaderVisibility = *Val; | ||
| else | ||
| return reportError(Ctx, "Invalid value for ShaderVisibility"); | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. There is no need to initialized There was a problem hiding this comment. It looks like a bug when the structure is created here but then only partially initialized. We should probably have a comment here, or even write a zero to it with a comment that it will be overwritten later. |
||
|
|
||
| dxbc::RootConstants Constants; | ||
| if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 2)) | ||
| Constants.ShaderRegister = *Val; | ||
| else | ||
| return reportError(Ctx, "Invalid value for ShaderRegister"); | ||
|
|
||
| if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 3)) | ||
| Constants.RegisterSpace = *Val; | ||
| else | ||
| return reportError(Ctx, "Invalid value for RegisterSpace"); | ||
|
|
||
| if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 4)) | ||
| Constants.Num32BitValues = *Val; | ||
| else | ||
| return reportError(Ctx, "Invalid value for Num32BitValues"); | ||
|
|
||
| RSD.ParametersContainer.addParameter(Header, Constants); | ||
|
|
||
| return false; | ||
| } | ||
|
|
@@ -164,12 +165,12 @@ static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) { | |
| return reportValueError(Ctx, "RootFlags", RSD.Flags); | ||
| } | ||
|
|
||
| for (const llvm::mcdxbc::RootParameterInfo &Info : RSD.ParametersContainer) { | ||
| if (!dxbc::isValidShaderVisibility(Info.Header.ShaderVisibility)) | ||
| return reportValueError(Ctx, "ShaderVisibility", | ||
| Info.Header.ShaderVisibility); | ||
|
|
||
| assert(dxbc::isValidParameterType(Info.Header.ParameterType) && | ||
| "Invalid value for ParameterType"); | ||
| } | ||
|
|
||
|
|
@@ -287,22 +288,26 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M, | |
| OS << indent(Space) << "Version: " << RS.Version << "\n"; | ||
| OS << indent(Space) << "RootParametersOffset: " << RS.RootParameterOffset | ||
| << "\n"; | ||
| OS << indent(Space) << "NumParameters: " << RS.ParametersContainer.size() | ||
| << "\n"; | ||
| Space++; | ||
| for (auto const &Info : RS.ParametersContainer) { | ||
| OS << indent(Space) << "- Parameter Type: " << Info.Header.ParameterType | ||
| << "\n"; | ||
| OS << indent(Space + 2) | ||
| << "Shader Visibility: " << Info.Header.ShaderVisibility << "\n"; | ||
| std::optional<mcdxbc::ParametersView> P = | ||
| RS.ParametersContainer.getParameter(&Info); | ||
| if (!P) | ||
| continue; | ||
| if (std::holds_alternative<const dxbc::RootConstants *>(*P)) { | ||
| auto *Constants = std::get<const dxbc::RootConstants *>(*P); | ||
| OS << indent(Space + 2) | ||
| << "Register Space: " << Constants->RegisterSpace << "\n"; | ||
| OS << indent(Space + 2) | ||
| << "Shader Register: " << Constants->ShaderRegister << "\n"; | ||
| OS << indent(Space + 2) | ||
| << "Num 32 Bit Values: " << Constants->Num32BitValues << "\n"; | ||
| } | ||
bogner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| Space--; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearer to call these
HeaderandLocationrather than abbreviating here