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

[0002] Root signatures: Fix example and reformat metadata schema #91

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

Conversation

damyanp
Copy link
Collaborator

@damyanp damyanp commented Oct 24, 2024

Move the example metadata to the main proposal section.

Update the layout of the schema so that it is ordered more from top-to-bottom and reformat to be easier to read.

Also fixed the metadata example to match the schema.

Copy link
Collaborator

@inbelic inbelic left a comment

Choose a reason for hiding this comment

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

I was able to follow and understand the descriptions well.

#### Root Signature

```LLVM
!3 = !{!4, !5, !6, !7 }
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
!3 = !{!4, !5, !6, !7 }
!3 = !{ !4, !5, !6, !7 }


```llvm
!dx.rootsignatures = !{!2} ; list of function/root signature pairs
!2 = !{ptr @main, !3 } ; function, root signature
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
!2 = !{ptr @main, !3 } ; function, root signature
!2 = !{ ptr @main, !3 } ; function, root signature

Not sure if needed, but everywhere else seems to have a space. There is another instance if this is the case on line 524


* i32: the root signature flags ([D3D12_ROOT_SIGNATURE_FLAGS](d3d12_root_signature_flags))


Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra line


Operands:

* i32: the root signature flags ([D3D12_ROOT_SIGNATURE_FLAGS](d3d12_root_signature_flags))
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
* i32: the root signature flags ([D3D12_ROOT_SIGNATURE_FLAGS](d3d12_root_signature_flags))
* i32: the root signature flags ([D3D12_ROOT_SIGNATURE_FLAGS][d3d12_root_signature_flags])

Discrepancy with the other flag links

* i32: number of descriptors in the range
* i32: base shader register
* i32: register space
* i32: descriptor range flgs ([D3D12_DESCRIPTOR_RANGE_FLAGS][d3d12_descriptor_range_flags])
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
* i32: descriptor range flgs ([D3D12_DESCRIPTOR_RANGE_FLAGS][d3d12_descriptor_range_flags])
* i32: descriptor range flags ([D3D12_DESCRIPTOR_RANGE_FLAGS][d3d12_descriptor_range_flags])

```

Operands:
* i32: Filter ([D3D12_FILTER](d3d12_filter))
Copy link
Collaborator

@inbelic inbelic Oct 25, 2024

Choose a reason for hiding this comment

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

These operands also use regular braces as opposed to square. Not sure which one is correct but I'll note it to double-check.

Copy link
Collaborator

@coopp coopp left a comment

Choose a reason for hiding this comment

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

Looks good to me., I like the additional cleanups here.

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.

3 participants