[NFC] Refactoring MCDXBC to support out of order storage of root parameters #137284
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -274,27 +274,39 @@ void DXContainerWriter::writeParts(raw_ostream &OS) { | |||||||||
| RS.StaticSamplersOffset = P.RootSignature->StaticSamplersOffset; | ||||||||||
|
|
||||||||||
| for (const auto &Param : P.RootSignature->Parameters) { | ||||||||||
| auto Header = dxbc::RootParameterHeader{Param.Type, Param.Visibility, | ||||||||||
| Param.Offset}; | ||||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||||
|
|
||||||||||
| switch (Param.Type) { | ||||||||||
| case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): | ||||||||||
| dxbc::RootConstants Constants; | ||||||||||
| Constants.Num32BitValues = Param.Constants.Num32BitValues; | ||||||||||
| Constants.RegisterSpace = Param.Constants.RegisterSpace; | ||||||||||
| Constants.ShaderRegister = Param.Constants.ShaderRegister; | ||||||||||
| RS.ParametersContainer.addParameter(Header, Constants); | ||||||||||
bogner marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| break; | ||||||||||
| case llvm::to_underlying(dxbc::RootParameterType::SRV): | ||||||||||
| case llvm::to_underlying(dxbc::RootParameterType::UAV): | ||||||||||
| case llvm::to_underlying(dxbc::RootParameterType::CBV): | ||||||||||
| if (RS.Version == 1) { | ||||||||||
| dxbc::RTS0::v1::RootDescriptor Descriptor; | ||||||||||
| Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace; | ||||||||||
| Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister; | ||||||||||
| RS.ParametersContainer.addParameter(Header, Descriptor); | ||||||||||
| } else { | ||||||||||
| dxbc::RTS0::v2::RootDescriptor Descriptor; | ||||||||||
| Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace; | ||||||||||
| Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister; | ||||||||||
| Descriptor.Flags = Param.Descriptor.getEncodedFlags(); | ||||||||||
| RS.ParametersContainer.addParameter(Header, Descriptor); | ||||||||||
| } | ||||||||||
|
There was a problem hiding this comment. This seems clearer than the implicit cast to from the dxbc::RTS0::v2::RootDescriptor Descriptor;
Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
if (RS.Version > 1)
Descriptor.Flags = Param.Descriptor.getEncodedFlags();
RS.ParametersContainer.addParameter(Header, Descriptor);aside: We should probably make the |
||||||||||
| break; | ||||||||||
| default: | ||||||||||
| // Handling invalid parameter type edge case. We intentionally let | ||||||||||
| // obj2yaml/yaml2obj parse and emit invalid dxcontainer data, in order | ||||||||||
| // for that to be used as a testing tool more effectively. | ||||||||||
| RS.ParametersContainer.addInfo(Header, -1); | ||||||||||
spall marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. I'm not sure, but do you think it's worth adding a There was a problem hiding this comment. I thinks that is a good idea. It can make the code more consistent. And reduce a little of the confusion regarding this edge case. |
||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| RS.write(OS); | ||||||||||
|
|
||||||||||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,31 +75,34 @@ static bool parseRootConstants(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD, | |
| if (RootConstantNode->getNumOperands() != 5) | ||
| return reportError(Ctx, "Invalid format for RootConstants Element"); | ||
|
|
||
| dxbc::RootParameterHeader Header; | ||
| // this will be properly calculated when writing it. | ||
| Header.ParameterOffset = 0; | ||
|
There was a problem hiding this comment. I think this wording might be a little bit too brief. How about "The parameter offset doesn't matter here - we recalculate it during serialization" |
||
| 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"); | ||
|
|
||
| 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 +167,12 @@ static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) { | |
| return reportValueError(Ctx, "RootFlags", RSD.Flags); | ||
| } | ||
|
|
||
| for (const 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,33 +290,40 @@ 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 (size_t I = 0; I < RS.ParametersContainer.size(); I++) { | ||
| const auto &[Type, Loc] = | ||
| RS.ParametersContainer.getTypeAndLocForParameter(I); | ||
| const dxbc::RootParameterHeader Header = | ||
| RS.ParametersContainer.getHeader(I); | ||
|
|
||
| OS << indent(Space) << "- Parameter Type: " << Type << "\n"; | ||
| OS << indent(Space + 2) | ||
| << "Shader Visibility: " << Header.ShaderVisibility << "\n"; | ||
|
|
||
| switch (Type) { | ||
| case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): { | ||
| const dxbc::RootConstants &Constants = | ||
| RS.ParametersContainer.getConstant(Loc); | ||
| 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"; | ||
| } | ||
| } | ||
| Space--; | ||
| } | ||
| OS << indent(Space) << "NumStaticSamplers: " << 0 << "\n"; | ||
| OS << indent(Space) << "StaticSamplersOffset: " << RS.StaticSamplersOffset | ||
| << "\n"; | ||
|
|
||
| Space--; | ||
| // end root signature header | ||
| } | ||
| return PreservedAnalyses::all(); | ||
| } | ||
|
|
||
|
|
||
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.
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