Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metadata schema to root signature #12

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

python3kgae
Copy link
Collaborator

This commit adds a metadata schema to the root signature.
This schema is used to describe the root signature in llvm IR.

This commit adds a metadata schema to the root signature.
This schema is used to describe the root signature in llvm IR.
proposals/0002-root-signature-in-clang.md Outdated Show resolved Hide resolved
proposals/0002-root-signature-in-clang.md Outdated Show resolved Hide resolved

| RootFlag | Parameters | StaticSamplers |
|----------|-------------------------------------------------|-------------------------|
| int | list of RootDescriptor/RootConstant/DescriptorTable | list of StaticSamplers |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't match the example. I think really it should be a list of parameters, and each parameter could be a RootDescriptor, RootConstant, DescriptorTable or StaticSampler. I don't think there's any requirement for the static samplers to come after the other parameters - the grammar doesn't enforce that, so I'm not sure why we would need to add that extra requirement to the metadata format?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the example.
The list will be null if no static samplers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why these two lists are separated out. They're not separate in the grammar, so why are we introducing that here?

If this is to match the d3d12 struct definitions, I don't think that's a goal here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It started from 2 different list in the string version of root signature.
And will end to 2 different list in the final serialized root signature.
It will be strange to merge them into one list and split later.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note also that in the example StaticSampler starts with a parameter type, "StaticSampler", precisely so that it can be intermingled with the other parameters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake, I though Static Sampler have to be at end of the root signature which is not true.
If we want to keep things in the same order of the string version of root signature.
Then make a one list make sense.

proposals/0002-root-signature-in-clang.md Outdated Show resolved Hide resolved
proposals/0002-root-signature-in-clang.md Outdated Show resolved Hide resolved
proposals/0002-root-signature-in-clang.md Outdated Show resolved Hide resolved
proposals/0002-root-signature-in-clang.md Outdated Show resolved Hide resolved
Comment on lines -409 to -422
Example for same root signature as above:

```llvm
; placeholder - update this once detailed design complete
!directx.rootsignatures = !{!2}
!2 = !{ptr @main, !3 }
!3 = !{ !4, !5, !6, !7 } ; reference 4 root parameters
!4 = !{ !"RootFlags", i32 1 } ; root flags, 1 is numeric value of flags
!5 = !{ !"RootCBV", i32 0, i32 1, i32 0, i32 0 } ; register 0, space 1, 0 = visiblity, 0 = flags
!6 = !{ !"StaticSampler", i32 1, i32 0, ... } ; register 1, space 0, (additional params omitted)
!7 = !{ !"DescriptorTable", i32 0, !8, !9 } ; 0 = visibility, 2 ranges,!8 and !9
!8 = !{ !"SRV", i32 0, i32 0, i32 -1, i32 0 } ; register 0, space 0, unbounded, flags 0
!9 = !{ !"UAV", i32 5, i32 1, i32 10, i32 0 } ; register 5, space 1, 10 descriptors, flags 0
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example should stay here since it's part of proposed solutions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this section is still in the detailed design part rather than the proposed solution.

Comment on lines -409 to -422
Example for same root signature as above:

```llvm
; placeholder - update this once detailed design complete
!directx.rootsignatures = !{!2}
!2 = !{ptr @main, !3 }
!3 = !{ !4, !5, !6, !7 } ; reference 4 root parameters
!4 = !{ !"RootFlags", i32 1 } ; root flags, 1 is numeric value of flags
!5 = !{ !"RootCBV", i32 0, i32 1, i32 0, i32 0 } ; register 0, space 1, 0 = visiblity, 0 = flags
!6 = !{ !"StaticSampler", i32 1, i32 0, ... } ; register 1, space 0, (additional params omitted)
!7 = !{ !"DescriptorTable", i32 0, !8, !9 } ; 0 = visibility, 2 ranges,!8 and !9
!8 = !{ !"SRV", i32 0, i32 0, i32 -1, i32 0 } ; register 0, space 0, unbounded, flags 0
!9 = !{ !"UAV", i32 5, i32 1, i32 10, i32 0 } ; register 5, space 1, 10 descriptors, flags 0
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this section is still in the detailed design part rather than the proposed solution.


| Property | Type | Type detail |
| ---------------- | ----- | -------------------------- |
| Filter | i32 | D3D12_FILTER |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a ParameterType / string / "StaticSampler" row.


| Property | Type | Type detail |
| -------------- | ----------------- | --------------------------------------------------- |
| Root elements | reference to list | list of D3D12_ROOT_SIGNATURE_FLAG/RootDescriptor/RootConstant/DescriptorTable/StaticSamplers |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| Root elements | reference to list | list of D3D12_ROOT_SIGNATURE_FLAG/RootDescriptor/RootConstant/DescriptorTable/StaticSamplers |
| Root elements | reference to list | list of root elements|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

2 participants