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.
This commit is contained in:
Erich Gubler 2024-08-23 13:45:19 -04:00
parent b81fcb4134
commit ea75a8ced4
3 changed files with 117 additions and 29 deletions

View File

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

View File

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

View File

@ -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<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))]
@ -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:
// <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);