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

WGSL: Implement diagnostic(…); directives and derivative_uniformity triggering rule #6148

Merged

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Aug 23, 2024

Connections

Resolution:

Dependencies:

Description

Recommended review experience is commit-by-commit. Each individual commit should pass CI!

This change implements a meaningful first iteration of support for diagnostic(…); directives; to wit:

  1. Define the essential state that Naga needs to track for compilation messages controlled by diagnostic(…) rules.
  2. Control Naga's current uniformity analysis errors via the derivative_uniformity rule. The current analysis is incorrect, and blocks users in issues like Feature request: disable uniformity analysis on shader creation #3135. This can give them a way forward.

Several things are out-of-scope:

  • There are more grammar rules where we should also @diagnostic(…) attributes, in order to conform fully with the WGSL spec. This PR does not attempt to handle all of these cases, to limit the complexity of this PR.

    The exhaustive list of standards-mandated parse sites for @diagnostic(…) not implemented in this PR is tracked in Support diagnostic filters in WGSL (i.e., diagnostic(…)) #5320.

  • Naga's current diagnostic model for shader compilation is too limited for us to comply with the spec.-mandated behavior with warnings. Naga offers only a Result to indicate operation success, or a single fatal error message. This conflicts with the Severity::Warning model introduced in this PR, because standard WGSL compilation might report multiple diagnostics in cases where parsing the shader module…

    1. …emits multiple warning items.
    2. …emits warnings in addition to a fatal error-level item.

    Because of this limitation, and because we want to limit complexity in this PR, diagnostic(warn, …) will not report a standard warning-level compilation item in cases that the standard dictates a warning-level compilation item. Instead, we will opt for log::warn!(…) entries. This is tracked as follow-up issues:

    New cases where the above is relevant:

    • unrecognized diagnostic rule names
    • uniformity analysis diagnostics emitted at a warning level

Testing

Current test plan:

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@ErichDonGubler ErichDonGubler added area: validation Issues related to validation, diagnostics, and error handling area: api Issues related to API surface area: cts Issues stemming from the WebGPU Conformance Test Suite naga Shader Translator area: naga front-end lang: WGSL WebGPU Shading Language labels Aug 23, 2024
@ErichDonGubler ErichDonGubler self-assigned this Aug 23, 2024
@ErichDonGubler ErichDonGubler changed the title WIP: MVP for diagnostic directives and attributes WIP: MVP for diagnostic rules Aug 23, 2024
@jimblandy

This comment was marked as resolved.

@jimblandy

This comment was marked as resolved.

@ErichDonGubler

This comment was marked as resolved.

@jimblandy

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler requested a review from a team as a code owner November 5, 2024 03:28
@ErichDonGubler
Copy link
Member Author

@jimblandy: This is ready for a full review!

@ErichDonGubler
Copy link
Member Author

Just validated that tests written in the (revised) Testing portion of the OP work as expected when consumed by Firefox. 👏🏻

CHANGELOG.md Outdated Show resolved Hide resolved
@ErichDonGubler

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler changed the title WGSL: Parse diagnostic(…); directives WGSL: Implement diagnostic(…); directives and derivative_uniformity triggering rule Nov 8, 2024
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

As far as I can see, this never actually populates naga::Module::diagnostic_filters or diagnostic_filter_leaf with what was parsed by the WGSL front end. The PR description says this is supposed to control how validation reports FunctionError::NonUniformControlFlow, so I would have expected the diagnostics to have been passed through to the Module.

If this is correct, then that also indicates some missing tests.

@ErichDonGubler
Copy link
Member Author

ErichDonGubler commented Nov 9, 2024

@jimblandy: Ah, it seems I'd missed a couple of things:

  1. Needed to remove DISABLE_UNIFORMITY_REQ_FOR_FRAGMENT_STAGE, which I've squashed into 2841f18. I added coverage to the existing uniform_control_flow test to check that one of the offending

    It would be much better if I had a WGSL snippet in hand that definitively fails, both for an end-to-end test and something for the CHANGELOG, but...we've struggled to find one, and I think the two hours I spent trying to figure it out needs to be price enough.

  2. Needed to wire up the transfer of state from a TranslationUnit into a Module, which I've squashed in f950c59. I added a test to ensure that a module's IR actually contains a parsed diagnostic rule.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Okay, this looks good. Just a few comments.

naga/src/valid/handles.rs Outdated Show resolved Hide resolved
naga/src/valid/analyzer.rs Show resolved Hide resolved
This resolves ambiguity that will be introduced in the subsequent commit
that adds `impl From<ConflictingDiagnosticRuleError> for naga::Error`.
- Remove `DISABLE_UNIFORMITY_REQ_FOR_FRAGMENT_STAGE`.
- Add `CHANGELOG` entry.
- Add coverage to `naga`'s `valid::analyzer::uniform_control_flow` test.
@ErichDonGubler ErichDonGubler enabled auto-merge (rebase) November 12, 2024 13:37
@ErichDonGubler ErichDonGubler merged commit ea75a8c into gfx-rs:trunk Nov 12, 2024
27 checks passed
@ErichDonGubler ErichDonGubler deleted the wgsl-diagnostic-directives branch November 12, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API surface area: cts Issues stemming from the WebGPU Conformance Test Suite area: naga front-end area: validation Issues related to validation, diagnostics, and error handling lang: WGSL WebGPU Shading Language naga Shader Translator
Projects
Status: Done
2 participants