-
Notifications
You must be signed in to change notification settings - Fork 52
[0052] Finalize Experimental DXIL Op proposal #729
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
Conversation
llvm-beanz
left a comment
There was a problem hiding this 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.
Flakebi
left a comment
There was a problem hiding this 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.
Co-authored-by: Chris B <[email protected]> Co-authored-by: Damyan Pepper <[email protected]>
|
@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. |
tex3d
left a comment
There was a problem hiding this 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.
Co-authored-by: Tex Riddell <[email protected]>
|
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 |
There was a problem hiding this comment.
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.
Presents the final proposed solution for experimental DXIL Ops which is to accept the 16bit FeatureID solution but to restrict the feature IDs to
0x0000and0x8000