From bdf774aa8bd6914685a993ba0bcdc4efdc0ee1bb Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 24 Aug 2021 17:44:16 -0700 Subject: [PATCH] Rename `BoundsCheckPolicy::UndefinedBehavior` to `Unchecked`. When we are leaning on robust buffer access to do the job for us, there's no undefined behavior going on. So `UndefinedBehavior` suggests people are doing something reckless even if they're not. The policy just says what Naga is doing, and it shouldn't pretend to say what the rest of the system is doing. --- cli/src/main.rs | 6 +++--- src/back/mod.rs | 26 +++++++++++++++++++------- src/back/spv/image.rs | 4 ++-- src/back/spv/index.rs | 2 +- tests/snapshots.rs | 4 ++-- 5 files changed, 27 insertions(+), 15 deletions(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index 372a8303bf..f0ddbbd47c 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -13,13 +13,13 @@ struct Args { /// what policy to use for index bounds checking for arrays, vectors, and /// matrices. /// - /// May be `Restrict`, `ReadZeroSkipWrite`, or `UndefinedBehavior` + /// May be `Restrict`, `ReadZeroSkipWrite`, or `Unchecked` #[argh(option)] index_bounds_check_policy: Option, /// what policy to use for texture bounds checking. /// - /// May be `Restrict`, `ReadZeroSkipWrite`, or `UndefinedBehavior` + /// May be `Restrict`, `ReadZeroSkipWrite`, or `Unchecked` #[argh(option)] image_bounds_check_policy: Option, @@ -62,7 +62,7 @@ impl FromStr for BoundsCheckPolicyArg { Ok(Self(match s.to_lowercase().as_str() { "restrict" => BoundsCheckPolicy::Restrict, "readzeroskipwrite" => BoundsCheckPolicy::ReadZeroSkipWrite, - "undefinedbehavior" => BoundsCheckPolicy::UndefinedBehavior, + "unchecked" => BoundsCheckPolicy::Unchecked, _ => { return Err(format!( "Invalid value for --index-bounds-check-policy: {}", diff --git a/src/back/mod.rs b/src/back/mod.rs index 7ac135f9d1..1ad49e3483 100644 --- a/src/back/mod.rs +++ b/src/back/mod.rs @@ -105,23 +105,35 @@ impl<'a> FunctionCtx<'_> { } } -/// How should code generated by Naga do indexing bounds checks? +/// How should code generated by Naga do bounds checks? /// /// When a vector, matrix, or array index is out of bounds—either negative, or /// greater than or equal to the number of elements in the type—WGSL requires /// that some other index of the implementation's choice that is in bounds is /// used instead. (There are no types with zero elements.) /// +/// Similarly, when out-of-bounds coordinates, array indices, or sample indices +/// are presented to the WGSL `textureLoad` and `textureStore` operations, the +/// operation is redirected to do something safe. +/// /// Different users of Naga will prefer different defaults: /// /// - When used as part of a WebGPU implementation, the WGSL specification -/// requires the `Restrict` behavior. +/// requires the `Restrict` behavior for array, vector, and matrix accesses, +/// and either the `Restrict` or `ReadZeroSkipWrite` behaviors for texture +/// accesses. /// /// - When used by the `wgpu` crate for native development, `wgpu` selects /// `ReadZeroSkipWrite` as its default. /// -/// - Naga's own default is `UndefinedBehavior`, so that shader translations +/// - Naga's own default is `Unchanged`, so that shader translations /// are as faithful to the original as possible. +/// +/// Sometimes the underlying hardware and drivers can perform bounds checks +/// themselves, in a way that performs better than the checks Naga would inject. +/// If you're using native checks like this, then having Naga inject its own +/// checks as well would be redundant, and the `Unchecked` policy is +/// appropriate. #[derive(Clone, Copy, Debug)] pub enum BoundsCheckPolicy { /// Replace out-of-bounds indexes with some arbitrary in-bounds index. @@ -135,16 +147,16 @@ pub enum BoundsCheckPolicy { /// Out-of-bounds reads return zero, and writes have no effect. ReadZeroSkipWrite, - /// Out-of-bounds indexes are undefined behavior. Generate the fastest code + /// Naga adds no checks to indexing operations. Generate the fastest code /// possible. This is the default for Naga, as a translator, but consumers /// should consider defaulting to a safer behavior. - UndefinedBehavior, + Unchecked, } -/// The default `BoundsCheckPolicy` is `UndefinedBehavior`. +/// The default `BoundsCheckPolicy` is `Unchecked`. impl Default for BoundsCheckPolicy { fn default() -> Self { - BoundsCheckPolicy::UndefinedBehavior + BoundsCheckPolicy::Unchecked } } diff --git a/src/back/spv/image.rs b/src/back/spv/image.rs index cf9cf802a7..0a93e735af 100644 --- a/src/back/spv/image.rs +++ b/src/back/spv/image.rs @@ -758,7 +758,7 @@ impl<'w> BlockContext<'w> { block, &access, )?, - crate::back::BoundsCheckPolicy::UndefinedBehavior => access.generate( + crate::back::BoundsCheckPolicy::Unchecked => access.generate( &mut self.writer.id_gen, coordinates.value_id, level_id, @@ -1146,7 +1146,7 @@ impl<'w> BlockContext<'w> { &write, )?; } - crate::back::BoundsCheckPolicy::UndefinedBehavior => { + crate::back::BoundsCheckPolicy::Unchecked => { write.generate( &mut self.writer.id_gen, coordinates.value_id, diff --git a/src/back/spv/index.rs b/src/back/spv/index.rs index 5176dd0d54..a6cf6bfb8f 100644 --- a/src/back/spv/index.rs +++ b/src/back/spv/index.rs @@ -347,7 +347,7 @@ impl<'w> BlockContext<'w> { BoundsCheckPolicy::ReadZeroSkipWrite => { self.write_index_comparison(base, index, block)? } - BoundsCheckPolicy::UndefinedBehavior => BoundsCheckResult::Computed(self.cached[index]), + BoundsCheckPolicy::Unchecked => BoundsCheckResult::Computed(self.cached[index]), }) } diff --git a/tests/snapshots.rs b/tests/snapshots.rs index f92ddebdcd..320e311e92 100644 --- a/tests/snapshots.rs +++ b/tests/snapshots.rs @@ -174,14 +174,14 @@ fn write_output_spv( } else if params.bounds_check_read_zero_skip_write { naga::back::BoundsCheckPolicy::ReadZeroSkipWrite } else { - naga::back::BoundsCheckPolicy::UndefinedBehavior + naga::back::BoundsCheckPolicy::Unchecked }, image_bounds_check_policy: if params.image_bounds_check_restrict { naga::back::BoundsCheckPolicy::Restrict } else if params.image_bounds_check_read_zero_skip_write { naga::back::BoundsCheckPolicy::ReadZeroSkipWrite } else { - naga::back::BoundsCheckPolicy::UndefinedBehavior + naga::back::BoundsCheckPolicy::Unchecked }, ..spv::Options::default() };