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

Back out unnecessary OMM flag validation and SM 6.9 dependency #360

Open
tex3d opened this issue Dec 18, 2024 · 1 comment
Open

Back out unnecessary OMM flag validation and SM 6.9 dependency #360

tex3d opened this issue Dec 18, 2024 · 1 comment
Labels
active proposal Issues relating to active proposals Design Meeting Agenda item for the design meeting

Comments

@tex3d
Copy link
Collaborator

tex3d commented Dec 18, 2024

Which proposal does this relate to?

When lowering to DXIL intrinsics, we will mask the flags using the legal mask
for the shader target. This will prevent undefined behavior if invalid flags
were specified and either warnings were ignored, or the flags were not a known
constant value during semantic analysis.

Describe the issue or outstanding question.
Before this proposal, unknown RayFlags had no diagnostics, masking, or validation. If an unknown flag was used, the DXR spec says that constitutes "undefined behavior", and that any unrecognized flag is not guaranteed to be available when reading the RayFlag state back.

This proposal adds error diagnostics for static use of new flags when SM < 6.9, warnings for use of the enum values, masking the RayFlags when generating the DXIL op to prevent use of any unknown flags for the current shader model, and tests for such.

After discussions, it is felt that the proposed change is overly restrictive and may add an unnecessary mask operation. Without this restriction, the new flags could be used even with a prior shader model on a device that supports OMM, without any real dependency on SM 6.9.

This has implications for the proposal to use availability diagnostics on the new flag values as well.

We propose to remove these extra diagnostics, masking, validation, and testing from the proposal. The new flags will be usable on prior shader models as long as the runtime and device recognize the flags, even if OMM is unsupported, with undefined behavior otherwise. Device support for OMM or support for SM 6.9 is sufficient to guarantee that a device will accept the new flags without undefined behavior, without requiring that the shader is compiled to SM 6.9.

However, for RayQuery, it's proposed that a new template argument be added, which will require a new parameter to a new version of the AllocateRayQuery DXIL op. This will still require a new version of DXIL, and thus require SM 6.9 to use.

@tex3d tex3d added active proposal Issues relating to active proposals needs-triage labels Dec 18, 2024
@llvm-beanz llvm-beanz added this to the Shader Model 6.9 milestone Dec 19, 2024
@llvm-beanz llvm-beanz added Design Meeting Agenda item for the design meeting and removed needs-triage labels Dec 19, 2024
@llvm-beanz llvm-beanz moved this to Triaged in HLSL Triage Dec 19, 2024
@llvm-beanz
Copy link
Collaborator

A user on the DirectX Discord's #hlsl recently said:

in any case HLSL will accept a lot of nonsense that does nothing, so you do need to make sure you understand what exactly you're writing

This is the state of our language today.

From the brief discussion we had during triage today, IMO there are places we could relax from what we put in the spec (dynamic enforcement might be overkill), but we should not fully remove this validation.

Just because something unsupported works if you hold it just right, doesn't mean it is good design, or that it is ergonomic for our users. I believe we should strive to make HLSL less magic, and less unintuitive, and that means making the language and compiler stricter about what we accept.

Allowing a user to explicitly use a flag in a shader model where it is not supported, is a prime example of letting a user write code that they think does one thing, when it actually does something different. This is exactly the kind of unintuitive experience we should be shielding our users from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
active proposal Issues relating to active proposals Design Meeting Agenda item for the design meeting
Projects
Status: No status
Status: Triaged
Development

No branches or pull requests

2 participants