Skip to content

Conversation

@V-FEXrt
Copy link
Collaborator

@V-FEXrt V-FEXrt commented Nov 12, 2025

Presents the final proposed solution for experimental DXIL Ops which is to accept the 16bit FeatureID solution but to restrict the feature IDs to 0x0000 and 0x8000

@V-FEXrt V-FEXrt requested review from llvm-beanz and tex3d November 12, 2025 18:45
Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

I've commented on some spelling errors, but I think the core of this is good.

One concern I do have is that we've put a lot of words into this proposal that make it a lot less clear to come in with fresh eyes, read the proposal and understand exactly what was decided by it. This proposal isn't completely unique, but I think we've gotten to this position because there was extensive discussion about the alternatives.

We'll need to consider carefully in the future how we can balance keeping historical context but also being succinct in the technical documentation so that the record of what we're building is concise and digestible.

Copy link

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

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

Doing a fresh read, I agree with @llvm-beanz, that a description of the actual accepted state is missing.
A final summary under “Proposed Solution” would be good to read and also to remove any ambiguities that come from the current description (which reads like “merge these two proposals in your head and add this third restricition”).
The descriptions are not long (the “Pros” would not belong into “Proposed Solutions”), the problem is that it’s scattered and contains two “diffs” that you need to apply in your head.

V-FEXrt and others added 2 commits November 13, 2025 10:37
@V-FEXrt
Copy link
Collaborator Author

V-FEXrt commented Nov 13, 2025

@Flakebi @llvm-beanz I simplified the "Accepted Solution" section to just state the facts of what we are doing as an independent proposal. Added a section just below on why that was the accepted proposal. Should be a lot easier now to just look at it to get an idea of the intended spec change.

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.

Looking good.

I pointed out a couple typos, some suggestions, and additional comments.

Nothing serious or blocking acceptance and merging.

@V-FEXrt V-FEXrt merged commit af1f840 into microsoft:main Nov 13, 2025
4 checks passed
@github-project-automation github-project-automation bot moved this to Triaged in HLSL Triage Nov 13, 2025
@Flakebi
Copy link

Flakebi commented Nov 14, 2025

Great, thanks for adding that. It’s much clearer now!

The top 16 bits of the opcode shall be used to partition the opcode into ~64k
partitions each with ~64k opcodes. The top 16 bits of the opcode are called
the `FeatureID` and the only valid FeatureIDs are `0x0000` and `0x8000`.
Within a given partition, opcodes must be contiguous. When opcodes are retired
Copy link
Collaborator

Choose a reason for hiding this comment

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

opcodes must be contiguous

I don't think this is technically true. The fact that DXC requires reserving opcodes is an oddity of DXC, but we could leave gaps, the only reason not to do that is that it has an undue cost to DXC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Triaged

Development

Successfully merging this pull request may close these issues.

5 participants