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.
gfx-rs · Nov 12, 2024 · ea75a8c · ea75a8c
1 parent b81fcb4
commit ea75a8c
Show file tree

Hide file tree

Showing 3 changed files with 117 additions and 29 deletions.
diff --git 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 <https://github.com/gfx-rs/wgpu/issues/4369>) this snippet would emit a uniformity error:
+
+```wgsl
+@group(0) @binding(0) var s : sampler;
+@group(0) @binding(2) var tex : texture_2d<f32>;
+@group(1) @binding(0) var<storage, read> ro_buffer : array<f32, 4>;
+
+@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<f32>;
+@group(1) @binding(0) var<storage, read> ro_buffer : array<f32, 4>;
+
+@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 <https://github.com/gfx-rs/wgpu/issues/5320>. We expect that rules in `fn` attribute position will be relaxed shortly (see <https://github.com/gfx-rs/wgpu/pull/6353>), 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 <https://github.com/gfx-rs/wgpu/issues/6458>.
+- 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., <https://github.com/gfx-rs/wgpu/issues/3135>) before we took significant effort to fix this (tracked in <https://github.com/gfx-rs/wgpu/issues/6496>).
+
+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
@@ -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<E>(
         self,
         err: E,
@@ -101,7 +100,6 @@ impl FilterableTriggeringRule {
     ///
     /// See <https://www.w3.org/TR/WGSL/#filterable-triggering-rules> 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<Handle<Self>>,
         arena: &Arena<Self>,

diff --git 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,19 +16,15 @@ use std::ops;
 
 pub type NonUniformResult = Option<Handle<crate::Expression>>;
 
-// 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))]
     #[cfg_attr(feature = "deserialize", derive(serde::Deserialize))]
     #[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:
+                                    // <https://github.com/gfx-rs/wgpu/issues/6458>
+                                    |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);