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

[SM6.8] Add proposal for DXIL 1.8 #77

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

Conversation

llvm-beanz
Copy link
Collaborator

This change proposes an abbreviated DXIL 1.8 specification. We should fill this out with more detail as appropriate, but this should document the basic bits that we need to verify the compiler implementation.

This change proposes an abbreviated DXIL 1.8 specification. We should
fill this out with more detail as appropriate, but this should document
the basic bits that we need to verify the compiler implementation.
@llvm-beanz llvm-beanz added this to the Shader Model 6.8 milestone Aug 17, 2023
@llvm-beanz llvm-beanz added the Requires Shader Model Feature requests that require DXIL and Shader Model updates label Aug 17, 2023
@jeremyong
Copy link

While a new shader model is introduced, do you think it might be possible to also deprecate constant buffer layout usage (with a flag) at the same time? Or would that introduce too much risk to the shader model release? I'm thinking the later that gets pushed out, the further into the future we'd have to maintain all that legacy cbuffer layout code :)

This updates the new barrier instruction signatures.
@llvm-beanz
Copy link
Collaborator Author

While a new shader model is introduced, do you think it might be possible to also deprecate constant buffer layout usage (with a flag) at the same time? Or would that introduce too much risk to the shader model release? I'm thinking the later that gets pushed out, the further into the future we'd have to maintain all that legacy cbuffer layout code :)

Unfortunately, the non-legacy cbuffer layout has lots of bugs and fixing those is out of scope for SM 6.8 (we're too late in the process). It is on our mind as a change we want to get resolved for the future, but it will miss SM 6.8

Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

I don't know if anything is missing, but whatevber might be can be added later.

├───────────────────────────────┼────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│allocateNodeOutputRecord │ 238 │%dx.types.NodeRecordHandle @dx.op.allocateNodeOutputRecords(i32, %dx.types.NodeHandle, i32, i1) │
├───────────────────────────────┼────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│getNodeRecordPtr │ 239 │<type> addrspace(6)* @dx.op.getNodeRecordPtr.<type>(i32, %dx.types.NodeRecordHandle, i32) │
Copy link
Member

Choose a reason for hiding this comment

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

We should document the new address space somewhere. Glancing at DXIL.rst, we don't document two (4 and 5) that we don't really use as-is.

ThreadNodeOutputRecords = (uint32_t)NodeIOFlags::ReadWrite |
(uint32_t)NodeIOFlags::Output |
(uint32_t)NodeIOFlags::ThreadRecord,
};
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be more easily consumable as a table either with a single field that lists the flags enabled or maybe even with a column for each flag.

Unlike memory operations in earlier versions of DXIL, reading and writing node
record memory uses LLVM's load and store instructions. DXIL 1.8 reserves address
space 6 for node memory. The `getNodeRecordPtr` DXIL operation returns a pointer
to node record memory in address space 6.
Copy link
Member

Choose a reason for hiding this comment

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

I see now this addresses the space. I feel like maybe this would be more useful higher anyway and it would make the appearance of address space 6 in that table less surprising as well as the absence of some expected intrinsics.

This updates the DXIL 1.8 additions document with the expected changes
to SM 6.8 features.
Nick Feeney
Amar Patel
Tex Riddell
Greg Roth
Copy link
Member

Choose a reason for hiding this comment

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

You may want to put this into a list with asterisks or find some other way to separate them. As written, these will all be on the same line, which makes it hard to tell where one name stops and the next starts.


### New DXIL Metadata

Node shader entry metadata can contain the following tagged entries:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel we should normalize language here for tag,value lists. This particular list are tags used in the optional extended properties MDList for an entry point, which also needs to be made clear.


The `kDxilNodeInputsTag` and `kDxilNodeOutputsTag` values are lists of node
metadata entries, where each node is an MDList of sub metadata entries based on
the following index table:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is another tag,value MDList. The values under Index here are not indices of an MDList, but the tag constant MDList operands preceding the value operand. As mentioned above, our language should be consistent across the tag,value MDList entries.

└─────────────────────────────────────────┴──────────┴──────────────────────────┘
```

`kDxilNodeRecordType` is a tagged metadata list based on the following tags:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is better, but perhaps we should link to a common term introduced and explained earlier for all of these.

Comment on lines +104 to +130
┌─────────────────────────────────────────┬──────────┬──────────────────────────┐
│ Tag │ Constant │ Value │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNumThreadsTag │ 4 │MDList: (i32, i32, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeLaunchTypeTag │ 13 │MDList: (i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeIsProgramEntryTag │ 14 │MDList: (i1) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeIdTag │ 15 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeLocalRootArgumentsTableIndexTag │ 16 │MDList: (i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilShareInputOfTag │ 17 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeDispatchGridTag │ 18 │MDList: (i32, i32, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxRecursionDepthTag │ 19 │MDList: (i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeInputsTag │ 20 │MDList: (MDList[]) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeOutputsTag │ 21 │MDList: (MDList[]) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxDispatchGridTag │ 22 │MD list: (i32, i32, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilRangedWaveSize │ 23 │MD list: (i32, i32, i32) │
└─────────────────────────────────────────┴──────────┴──────────────────────────┘
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Value column doesn't look right. Each value seems to imply that it points to another MDList containing a value or values. This isn't the case. The single values here are operands in-line after the associated tag that identifies the next operand. The multiple values are another MDList, containing that set of values.

The node inputs/outputs is confusing, as it appears to me like an MDList containing a single MDList operand with some set of entries (which might imply that this set of entries are the inputs or outputs).

The meaning/encoding of most of the metadata values is not described.

Suggested change
┌─────────────────────────────────────────┬──────────┬──────────────────────────┐
│ Tag │ Constant │ Value │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNumThreadsTag │ 4 │MDList: (i32, i32, i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeLaunchTypeTag │ 13 │MDList: (i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeIsProgramEntryTag │ 14 │MDList: (i1)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeIdTag │ 15 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeLocalRootArgumentsTableIndexTag │ 16 │MDList: (i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilShareInputOfTag │ 17 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeDispatchGridTag │ 18 │MDList: (i32, i32, i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxRecursionDepthTag │ 19 │MDList: (i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeInputsTag │ 20 │MDList: (MDList[])
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeOutputsTag │ 21 │MDList: (MDList[])
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxDispatchGridTag │ 22 │MD list: (i32, i32, i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilRangedWaveSize │ 23 │MD list: (i32, i32, i32)
└─────────────────────────────────────────┴──────────┴──────────────────────────┘
┌─────────────────────────────────────────┬──────────┬──────────────────────────────
│ Tag │ Constant │ Value
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNumThreadsTag │ 4 │MDList: (i32 x, i32 y, i32 z)
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNodeLaunchTypeTag │ 13 │i32
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNodeIsProgramEntryTag │ 14 │i1
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNodeIdTag │ 15 │MDList: (MDString, i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNodeLocalRootArgumentsTableIndexTag │ 16 │i32
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilShareInputOfTag │ 17 │MDList: (MDString, i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNodeDispatchGridTag │ 18 │MDList: (i32 x, i32 y, i32 z)
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNodeMaxRecursionDepthTag │ 19 │i32
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNodeInputsTag │ 20 │MDList: (MDList[], ...)
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNodeOutputsTag │ 21 │MDList: (MDList[], ...)
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilNodeMaxDispatchGridTag │ 22 │MDList: (i32 x, i32 y, i32 z)
├─────────────────────────────────────────┼──────────┼──────────────────────────────
│kDxilRangedWaveSize │ 23 │MDList: (i32 x, i32 y, i32 z)
└─────────────────────────────────────────┴──────────┴──────────────────────────────

Comment on lines +141 to +157
┌─────────────────────────────────────────┬──────────┬──────────────────────────┐
│ Tag │ Index │ Value │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeOutputIDTag │ 0 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeIOFlagsTag │ 1 │MDList: (i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeRecordTypeTag │ 2 │MDList: (MDList[]) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxRecordsTag │ 3 │MDList: (i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxRecordsSharedWithTag │ 4 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeOutputArraySizeTag │ 5 │MDList: (i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeAllowSparseNodesTag │ 6 │MDList: (i1) │
└─────────────────────────────────────────┴──────────┴──────────────────────────┘
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar issues with value as described in prior comment.

Suggested change
┌─────────────────────────────────────────┬──────────┬──────────────────────────┐
│ Tag │ Index │ Value │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeOutputIDTag │ 0 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeIOFlagsTag │ 1 │MDList: (i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeRecordTypeTag │ 2 │MDList: (MDList[])
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxRecordsTag │ 3 │MDList: (i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxRecordsSharedWithTag │ 4 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeOutputArraySizeTag │ 5 │MDList: (i32)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeAllowSparseNodesTag │ 6 │MDList: (i1)
└─────────────────────────────────────────┴──────────┴──────────────────────────┘
┌─────────────────────────────────────────┬──────────┬──────────────────────────┐
│ Tag │ Index │ Value │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeOutputIDTag │ 0 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeIOFlagsTag │ 1 │i32
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeRecordTypeTag │ 2 │MDList: (<tag-value list>)
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxRecordsTag │ 3 │i32
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeMaxRecordsSharedWithTag │ 4 │MDList: (MDString, i32) │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeOutputArraySizeTag │ 5 │i32
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeAllowSparseNodesTag │ 6 │i1
└─────────────────────────────────────────┴──────────┴──────────────────────────┘

┌─────────────────────────────────────────┬──────────┬──────────────────────────┐
│ Tag │ Constant │ Value │
├─────────────────────────────────────────┼──────────┼──────────────────────────┤
│kDxilNodeRecordSizeTag │ 0 │MDList: (i32) │
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
│kDxilNodeRecordSizeTag │ 0 │MDList: (i32)
│kDxilNodeRecordSizeTag │ 0 │i32

Copy link
Collaborator

@tex3d tex3d left a comment

Choose a reason for hiding this comment

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

I'm going to approve so we may add the document. However, there are a lot of deficiencies to address after we add the document, which should be captured as follow-up issues/tasks.

We should probably update the status to reflect the fact that we have already shipped this as well.

Besides the review feedback so far, there is also:

  • Missing feature details for: expanded-comparison-sampling and extended-command-info
  • Lacking descriptions for each new DXIL intrinsic and their parameters.
  • Lacking descriptions for new metadata
  • Lacking new feature flags and descriptions

There may be more missing that I'm not thinking of right now, but we can address that as we update the document and these things come up.

@llvm-beanz llvm-beanz added the Design Meeting Agenda item for the design meeting label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Meeting Agenda item for the design meeting Requires Shader Model Feature requests that require DXIL and Shader Model updates
Projects
Status: In Review
Status: In progress
Development

Successfully merging this pull request may close these issues.

4 participants