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

[OMM] Add AllocateRayQuery2 DXIL Op, add validation #7124

Merged
merged 20 commits into from
Mar 1, 2025

Conversation

bob80905
Copy link
Collaborator

@bob80905 bob80905 commented Feb 5, 2025

This PR adds AllocateRayQuery2 as a new DXIL Op, which is intended to be the DXIL Op that any rayquery object initialized with the 2nd optional template argument will be lowered to. Additionally, according to the spec here: https://github.com/microsoft/hlsl-specs/blob/main/proposals/0024-opacity-micromaps.md
there are new validation rules that should be run against the allocateRayQuery* DXIL Ops. This PR implements these validation rules, and tests them.

Fixes #7113

@bob80905 bob80905 requested a review from a team as a code owner February 5, 2025 01:56
Copy link
Contributor

github-actions bot commented Feb 5, 2025

✅ With the latest revision this PR passed the Python code formatter.

Copy link
Contributor

github-actions bot commented Feb 5, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@simoll
Copy link
Contributor

simoll commented Feb 6, 2025

I'll pull the new dxil opcode AllocateRayQuery2 into the opcode block reserved by:#7125

The opcode 258 is currently claimed by both PRs. This will allow us to merge more easily.

Copy link
Member

@damyanp damyanp left a comment

Choose a reason for hiding this comment

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

LGTM - some questions, maybe something you want to change as well.

You really should get a review from someone familiar with the DXC codebase.

Copy link
Contributor

@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.

See comments for details:

  • new op should not be in DXIL 1.8
  • wrong location for DXIL op call checking code
  • Some minor suggestions on validation rule naming

Copy link
Contributor

@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.

Looks good!

@bob80905 bob80905 merged commit 0a11435 into microsoft:main Mar 1, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[OMM] Add AllocateRayQuery2 DXIL Op
5 participants