Add proposal for root signature in clang. #193
Conversation
I'm not sure what to do for the future versions. |
My read of the IR representation that you're proposing is that it is pretty tightly linked to the binary encoding that gets put into the container file. I think we should decouple those representations. |
| !17 = !{i32 1, i32 0, i32 10, i32 0, i32 3} | ||
| !18 = !{!19, !20} ; static samplers. | ||
| !19 = !{i32 85, i32 1, i32 1, i32 1, float 0.000000e+00, i32 16, i32 4, i32 2, float 0.000000e+00, float 0x47EFFFFFE0000000, i32 1, i32 0, i32 0} | ||
| !20 = !{i32 21, i32 3, i32 1, i32 1, float 0.000000e+00, i32 16, i32 4, i32 2, float 0.000000e+00, float 0x47EFFFFFE0000000, i32 2, i32 0, i32 0} |
There was a problem hiding this comment.
IMO, this is a huge step in the right direction. I suspect there is some opportunity for further refinement, but this is much simpler to read and specify in the IR.
Can you document in more detail what each value in the metadata corresponds to? Even trying to map this to the structures from the structure-based approach is extremely tedious. I’d really like to see something like how DXIL.rst documents its metadata structures.
There was a problem hiding this comment.
I believe this comment has still not been addressed.
There was a problem hiding this comment.
Oh, I see in your update you mixed documenting the schema with the example. I think we should separate the documentation of the schema from the example. The example is helpful for driving conversation, but is something we should not include in the technical documentation.
There was a problem hiding this comment.
Added separate section for metadata format.
| struct ParsedRootSignature { | ||
| dxbc::RootSignature::ContainerRootSignatureDesc RSDesc; | ||
| llvm::SmallVector<RootSignature::ContainerRootParameter, 8> | ||
| RSParameters; | ||
| llvm::SmallVector<RootSignature::StaticSamplerDesc, 8> StaticSamplers; | ||
| llvm::SmallVector<RootSignature::ContainerDescriptorRange, 8> | ||
| DescriptorRanges; | ||
| llvm::SmallVector< | ||
| std::variant<RootSignature::ContainerRootConstants, | ||
| RootSignature::ContainerRootDescriptor, | ||
| RootSignature::ContainerRootDescriptorTable>, | ||
| 8> | ||
| RSParamExtras; | ||
| }; |
There was a problem hiding this comment.
I don't understand how this maps to root signatures. For example, how would these structs represent this root parameter:
DescriptorTable(
CBV(b1),
SRV(t1, numDescriptors = 8, flags = DESCRIPTORS_VOLATILE),
UAV(u1, numDescriptors = unbounded, flags = DESCRIPTORS_VOLATILE)),
ContainerRootDescriptorTable only has a NumDescriptorRanges field.
There was a problem hiding this comment.
Added comment for DescriptorRanges.
There was a problem hiding this comment.
Ah, I see. I don't understand why this design was chosen rather than a more direct one - eg where ContainerRootDescriptorTable might look like:
struct ContainerRootDescriptorTable {
SmallVector<ContainerDescriptorRange> Ranges;
};There was a problem hiding this comment.
It was started from final serialization format.
Updated to save Ranges in ContainerRootDescriptorTable .
| 2) Compiler must translate textual root signatures into binary encoded | ||
| formats. | ||
| 3) Compiler must support reading and writing binary encoded root | ||
| ignatures to and from DXContainer files. |
There was a problem hiding this comment.
Good catch. Fixed.
| The other mechanism is to create a standalone root signature blob, perhaps to | ||
| reuse it with a large set of shaders, saving space. There're shader model | ||
| rootsig_1_0 androotsig_1_1 to generate the standalone root signature blob. | ||
| The name of the define string is specified via the usual /E argument. |
There was a problem hiding this comment.
I am confused. I thought /E was for specifying the entrypoint on the commandline. DXC help shows this:
-E <value> Entry point name
There was a problem hiding this comment.
The target -Trootsig_1_0/1 is for compiling a root signature.
The macro name for the root signature string is treated as entry point name for this case.
|
|
||
| ``` | ||
|
|
||
| There's -setrootsignature option in DXC which attach root signature part to an existing input DXIL container. It is a DXIL container level operation. Not AST or llvm IR. |
There was a problem hiding this comment.
nit: This line exceeds the 80 col limit.
| The AST node serialization code might be looked like this: | ||
| ``` | ||
| void ASTDeclWriter::VisitHLSLRootSignatureDecl(HLSLRootSignatureDecl *D) { | ||
| VisitNamedDecl(D); | ||
| Record.AddSourceLocation(D->getKeywordLoc()); | ||
| hlsl::ParsedRootSignature &RS = D->getRootSignature(); | ||
| Record.push_back(RS.RSDesc.Version); | ||
| Record.push_back(RS.RSDesc.Flags); | ||
| for (const auto &P : ParsedRS.RSParameters) { | ||
| // push_back all the fields of root parameters. | ||
| } | ||
| for (const auto &SS : ParsedRS.StaticSamplers) { | ||
| // push_back all the fields of static samplers. | ||
| } | ||
| for (const auto &Range : ParsedRS.DescriptorRanges) { | ||
| // push_back all the descriptor ranges. | ||
| } | ||
|
|
||
| Code = serialization::DECL_HLSL_ROOT_SIGNATURE; | ||
| } | ||
|
|
||
| ``` |
There was a problem hiding this comment.
I think that this code is now out of date with the latest versions of the structs in this document.
I don't know that you need the code here. Are you saying that the root signature will the be serialized using standard LLVM bitcode?
There was a problem hiding this comment.
The pseudocode is also not correct. When writing arrays to bitcode (as with pretty much any binary serialization) you need to push the size of the array before the N elements so that when you decode you know how many elements to read.
| Because the RootSignature attribute in hlsl only have the string, it is easier | ||
| to add a StringArgument to save the string first. |
There was a problem hiding this comment.
it is easier to add a StringArgument to save the string first.
I don't understand what this is trying to say. Add a StringArgument to what?
There was a problem hiding this comment.
Updated this part.
| A HLSLRootSignatureDecl will be created and added to the HLSLRootSignatureAttr | ||
| for diagnostic and saved to the HLSLEntryRootSignatureAttr for clang |
There was a problem hiding this comment.
Why are there separate HLSLRootSignatureDecl and HLSLRootSignatureAttr nodes? What's the difference between them? Is this a normal pattern in Clang?
There was a problem hiding this comment.
HLSLRootSignatureDecl is shared between RootSignature attribute and local/global root signature.
HLSLRootSignatureAttr is only for the RootSignature attribute.
| A new AST node HLSLRootSignatureDecl will be added to represent the root | ||
| signature in the AST. | ||
| To avoid parsing root signature string twice, HLSLRootSignatureDecl will | ||
| save the parsed root siganture instead of the string. |
| The HLSLRootSignatureDecl will save StringLiteral instead StringRef for | ||
| diagnostic. |
There was a problem hiding this comment.
I don't understand the significance of using StringLiteral instead of StringRef here. Is there something about StringLiteral that makes diagnostics easier? What's the reason for the decision to use one over the other?
There was a problem hiding this comment.
StringLiteral has source location which will help to emit diagnostic.
| }; | ||
|
|
||
| ``` | ||
| only a HLSLRootSignatureDecl will be created. |
There was a problem hiding this comment.
How are global/local root signatures represented in the AST?
There was a problem hiding this comment.
As a HLSLRootSignatureDecl node.
| "MyHitGroup;MyMissShader" // Exports association | ||
| }; | ||
| ``` | ||
| The SubobjectToExportsAssociationDecl will contain a HLSLRootSignatureDecl and |
There was a problem hiding this comment.
Presumably something at some point validates that the subject name and the exports actually refer to things that exist. When does this happen?
HLSLRootSignatureDecl contains the full rootsignature definition string, doesn't it, not just the name of the root signature? Or does this AST node mean different things depending on the context it is referenced from?
There was a problem hiding this comment.
The name exist should be checked in Sema when creating the SubobjectToExportsAssociationDecl node. If cannot find the name, it should report error and SubobjectToExportsAssociationDecl will not be created.
HLSLRootSignatureDecl will have the name of the root signature (faked name for a root signature attribute) and a ParsedRootSignature object.
| MirrorOnce = 5 | ||
| }; | ||
|
|
||
| struct ContainerRootDescriptor { |
There was a problem hiding this comment.
Many of the structs defined below have initialized default values, but there are some with members that are missing values. Was this intentional?
Example:
ContainerRootDescriptor initializes RegisterSpace but not the others.
ContainerDescriptorRange is missing inits
There was a problem hiding this comment.
Not all members have default value.
Added the missing RegisterSpace init.
| a list of FunctionDecls. | ||
|
|
||
|
|
||
| The default root signature version number is 2 which maps to rootsig_1_1. |
There was a problem hiding this comment.
Seems like this should be higher up in the document. Talking about the different versions of root signatures. Are you calling this info out here because it is going to be written in the metadata at some point?
Also would the version also be stored in the deserialized version of the root signature?
There was a problem hiding this comment.
Cannot find a good place to add it. Do you have suggestion about it?
The version stored in deserialized/serialized version of root signature.
| HLSLRootSignature 'main.RS' | ||
|
|
||
| ``` | ||
| It is possible to avoid the root signature string in HLSLEntryRootSignatureAttr |
There was a problem hiding this comment.
I'm confused by this statement. Are you pointing out an optimization here? Leaving out / adding the root signature string in the printed output? Is this for diagnostics?
There was a problem hiding this comment.
I'm pointing out that print the root signature string for HLSLEntryRootSignatureAttr when dump AST is optional for implementation.
| * Save root signature string to AST. | ||
|
|
||
| Assuming the cost to parse root signature string is cheap, | ||
| HLSLRootSignatureDecl could follow common Clang approach which only save |
There was a problem hiding this comment.
Following the common clang approach is a good starting point. More investigation would be needed to really bottom out on how expensive string parsing is and how often it needs to be parsed. If it is parsed once into an object that can be passed to other places to be inspected that might not be too bad, but I don't fully understand how clang deals with strings in the AST yet. Maybe it stays as a string forever and travels around to consumers of the AST.
| ``` | ||
|
|
||
| Con: | ||
| Need to parse more than once. |
There was a problem hiding this comment.
Ahh.. So it will be parsed multiple times. I see.
|
Superseded by #209. |
Move discussion from llvm/llvm-project#83933