From ea75a8ced4381c04dd4cbc2a13d467e5cc9c6e25 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Fri, 23 Aug 2024 13:45:19 -0400 Subject: [PATCH] feat(wgsl-in): filter unif. analysis errors with `derivative_uniformity` - Remove `DISABLE_UNIFORMITY_REQ_FOR_FRAGMENT_STAGE`. - Add `CHANGELOG` entry. - Add coverage to `naga`'s `valid::analyzer::uniform_control_flow` test. --- CHANGELOG.md | 53 ++++++++++++++++++++- naga/src/diagnostic_filter.rs | 3 -- naga/src/valid/analyzer.rs | 90 +++++++++++++++++++++++++---------- 3 files changed, 117 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40245b55e..afaa5737a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,11 +40,62 @@ Bottom level categories: ## Unreleased +## Major changes + +### The `diagnostic(…);` directive is now supported in WGSL + +Naga now parses `diagnostic(…);` directives according to the WGSL spec. This allows users to control certain lints, similar to Rust's `allow`, `warn`, and `deny` attributes. For example, in standard WGSL (but, notably, not Naga yet—see ) this snippet would emit a uniformity error: + +```wgsl +@group(0) @binding(0) var s : sampler; +@group(0) @binding(2) var tex : texture_2d; +@group(1) @binding(0) var ro_buffer : array; + +@fragment +fn main(@builtin(position) p : vec4f) -> @location(0) vec4f { + if ro_buffer[0] == 0 { + // Emits a derivative uniformity error during validation. + return textureSample(tex, s, vec2(0.,0.)); + } + + return vec4f(0.); +} +``` + +…but we can now silence it with the `off` severity level, like so: + +```wgsl +// Disable the diagnosic with this… +diagnostic(off, derivative_uniformity); + +@group(0) @binding(0) var s : sampler; +@group(0) @binding(2) var tex : texture_2d; +@group(1) @binding(0) var ro_buffer : array; + +@fragment +fn main(@builtin(position) p : vec4f) -> @location(0) vec4f { + if ro_buffer[0] == 0 { + // Look ma, no error! + return textureSample(tex, s, vec2(0.,0.)); + } + + return vec4f(0.); +} +``` + +There are some limitations to keep in mind with this new functionality: + +- We do not yet support `diagnostic(…)` rules in attribute position (i.e., `@diagnostic(…) fn my_func { … }`). This is being tracked in . We expect that rules in `fn` attribute position will be relaxed shortly (see ), but the prioritization for statement positions is unclear. If you are blocked by not being able to parse `diagnostic(…)` rules in statement positions, please let us know in that issue, so we can determine how to prioritize it! +- Standard WGSL specifies `error`, `warning`, `info`, and `off` severity levels. These are all technically usable now! A caveat, though: warning- and info-level are only emitted to `stderr` via the `log` façade, rather than being reported through a `Result::Err` in Naga or the `CompilationInfo` interface in `wgpu{,-core}`. This will require breaking changes in Naga to fix, and is being tracked by . +- Not all lints can be controlled with `diagnostic(…)` rules. In fact, only the `derivative_uniformity` triggering rule exists in the WGSL standard. That said, Naga contributors are excited to see how this level of control unlocks a new ecosystem of configurable diagnostics. +- Finally, `diagnostic(…)` rules are not yet emitted in WGSL output. This means that `wgsl-in` → `wgsl-out` is currently a lossy process. We felt that it was important to unblock users who needed `diagnostic(…)` rules (i.e., ) before we took significant effort to fix this (tracked in ). + +By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148](https://github.com/gfx-rs/wgpu/pull/6148). + ### New Features #### Naga -- Parse `diagnostic(…)` directives, but don't implement any triggering rules yet. By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456). - Fix an issue where `naga` CLI would incorrectly skip the first positional argument when `--stdin-file-path` was specified. By @ErichDonGubler in [#6480](https://github.com/gfx-rs/wgpu/pull/6480). - Fix textureNumLevels in the GLSL backend. By @magcius in [#6483](https://github.com/gfx-rs/wgpu/pull/6483). - Implement `quantizeToF16()` for WGSL frontend, and WGSL, SPIR-V, HLSL, MSL, and GLSL backends. By @jamienicol in [#6519](https://github.com/gfx-rs/wgpu/pull/6519). diff --git a/naga/src/diagnostic_filter.rs b/naga/src/diagnostic_filter.rs index e599b32c6..602953ca4 100644 --- a/naga/src/diagnostic_filter.rs +++ b/naga/src/diagnostic_filter.rs @@ -48,7 +48,6 @@ impl Severity { /// Naga does not yet support diagnostic items at lesser severities than /// [`Severity::Error`]. When this is implemented, this method should be deleted, and the /// severity should be used directly for reporting diagnostics. - #[cfg(feature = "wgsl-in")] pub(crate) fn report_diag( self, err: E, @@ -101,7 +100,6 @@ impl FilterableTriggeringRule { /// /// See for a table of default /// severities. - #[allow(dead_code)] pub(crate) const fn default_severity(self) -> Severity { match self { FilterableTriggeringRule::DerivativeUniformity => Severity::Error, @@ -227,7 +225,6 @@ impl DiagnosticFilterNode { /// is found, return the value of [`FilterableTriggeringRule::default_severity`]. /// /// When `triggering_rule` is not applicable to this node, its parent is consulted recursively. - #[allow(dead_code)] pub(crate) fn search( node: Option>, arena: &Arena, diff --git a/naga/src/valid/analyzer.rs b/naga/src/valid/analyzer.rs index 91060dde3..e169ed1d0 100644 --- a/naga/src/valid/analyzer.rs +++ b/naga/src/valid/analyzer.rs @@ -6,7 +6,7 @@ //! - expression reference counts use super::{ExpressionError, FunctionError, ModuleInfo, ShaderStages, ValidationFlags}; -use crate::diagnostic_filter::DiagnosticFilterNode; +use crate::diagnostic_filter::{DiagnosticFilterNode, FilterableTriggeringRule}; use crate::span::{AddSpan as _, WithSpan}; use crate::{ arena::{Arena, Handle}, @@ -16,10 +16,6 @@ use std::ops; pub type NonUniformResult = Option>; -// Remove this once we update our uniformity analysis and -// add support for the `derivative_uniformity` diagnostic -const DISABLE_UNIFORMITY_REQ_FOR_FRAGMENT_STAGE: bool = true; - bitflags::bitflags! { /// Kinds of expressions that require uniform control flow. #[cfg_attr(feature = "serialize", derive(serde::Serialize))] @@ -27,8 +23,8 @@ bitflags::bitflags! { #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct UniformityRequirements: u8 { const WORK_GROUP_BARRIER = 0x1; - const DERIVATIVE = if DISABLE_UNIFORMITY_REQ_FOR_FRAGMENT_STAGE { 0 } else { 0x2 }; - const IMPLICIT_LEVEL = if DISABLE_UNIFORMITY_REQ_FOR_FRAGMENT_STAGE { 0 } else { 0x4 }; + const DERIVATIVE = 0x2; + const IMPLICIT_LEVEL = 0x4; } } @@ -828,7 +824,6 @@ impl FunctionInfo { /// Returns a `NonUniformControlFlow` error if any of the expressions in the block /// require uniformity, but the current flow is non-uniform. #[allow(clippy::or_fun_call)] - #[allow(clippy::only_used_in_recursion)] fn process_block( &mut self, statements: &crate::Block, @@ -852,8 +847,21 @@ impl FunctionInfo { && !req.is_empty() { if let Some(cause) = disruptor { - return Err(FunctionError::NonUniformControlFlow(req, expr, cause) - .with_span_handle(expr, expression_arena)); + let severity = DiagnosticFilterNode::search( + self.diagnostic_filter_leaf, + diagnostic_filter_arena, + FilterableTriggeringRule::DerivativeUniformity, + ); + severity.report_diag( + FunctionError::NonUniformControlFlow(req, expr, cause) + .with_span_handle(expr, expression_arena), + // TODO: Yes, this isn't contextualized with source, because + // the user is supposed to render what would normally be an + // error here. Once we actually support warning-level + // diagnostic items, then we won't need this non-compliant hack: + // + |e, level| log::log!(level, "{e}"), + )?; } } requirements |= req; @@ -1336,26 +1344,58 @@ fn uniform_control_flow() { }; { let block_info = info.process_block( - &vec![stmt_emit2, stmt_if_non_uniform].into(), + &vec![stmt_emit2.clone(), stmt_if_non_uniform.clone()].into(), &[], None, &expressions, &Arena::new(), ); - if DISABLE_UNIFORMITY_REQ_FOR_FRAGMENT_STAGE { - assert_eq!(info[derivative_expr].ref_count, 2); - } else { - assert_eq!( - block_info, - Err(FunctionError::NonUniformControlFlow( - UniformityRequirements::DERIVATIVE, - derivative_expr, - UniformityDisruptor::Expression(non_uniform_global_expr) - ) - .with_span()), - ); - assert_eq!(info[derivative_expr].ref_count, 1); - } + assert_eq!( + block_info, + Err(FunctionError::NonUniformControlFlow( + UniformityRequirements::DERIVATIVE, + derivative_expr, + UniformityDisruptor::Expression(non_uniform_global_expr) + ) + .with_span()), + ); + assert_eq!(info[derivative_expr].ref_count, 1); + + // Test that the same thing passes when we disable the `derivative_uniformity` + let mut diagnostic_filters = Arena::new(); + let diagnostic_filter_leaf = diagnostic_filters.append( + DiagnosticFilterNode { + inner: crate::diagnostic_filter::DiagnosticFilter { + new_severity: crate::diagnostic_filter::Severity::Off, + triggering_rule: FilterableTriggeringRule::DerivativeUniformity, + }, + parent: None, + }, + crate::Span::default(), + ); + let mut info = FunctionInfo { + diagnostic_filter_leaf: Some(diagnostic_filter_leaf), + ..info.clone() + }; + + let block_info = info.process_block( + &vec![stmt_emit2, stmt_if_non_uniform].into(), + &[], + None, + &expressions, + &diagnostic_filters, + ); + assert_eq!( + block_info, + Ok(FunctionUniformity { + result: Uniformity { + non_uniform_result: None, + requirements: UniformityRequirements::DERIVATIVE, + }, + exit: ExitFlags::empty() + }), + ); + assert_eq!(info[derivative_expr].ref_count, 2); } assert_eq!(info[non_uniform_global], GlobalUse::READ);