From 6a5b33fa2493022f0b51f5d1f5d10565247dedf6 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Wed, 17 Nov 2021 19:15:59 -0800 Subject: [PATCH] Move `BoundsCheckPolicy`/`Policies` into `proc`, from `back`. (#1537) --- cli/src/main.rs | 6 +- src/back/mod.rs | 111 ---------------------------------- src/back/spv/image.rs | 12 ++-- src/back/spv/index.rs | 23 ++----- src/back/spv/mod.rs | 5 +- src/proc/index.rs | 136 ++++++++++++++++++++++++++++++++++++++++++ src/proc/mod.rs | 2 +- tests/snapshots.rs | 4 +- 8 files changed, 157 insertions(+), 142 deletions(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index 4330d882d0..5f5d59f222 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -71,13 +71,13 @@ struct Args { /// Newtype so we can implement [`FromStr`] for `BoundsCheckPolicy`. #[derive(Debug, Clone, Copy)] -struct BoundsCheckPolicyArg(naga::back::BoundsCheckPolicy); +struct BoundsCheckPolicyArg(naga::proc::BoundsCheckPolicy); impl FromStr for BoundsCheckPolicyArg { type Err = String; fn from_str(s: &str) -> Result { - use naga::back::BoundsCheckPolicy; + use naga::proc::BoundsCheckPolicy; Ok(Self(match s.to_lowercase().as_str() { "restrict" => BoundsCheckPolicy::Restrict, "readzeroskipwrite" => BoundsCheckPolicy::ReadZeroSkipWrite, @@ -132,7 +132,7 @@ impl FromStr for GlslProfileArg { #[derive(Default)] struct Parameters { validation_flags: naga::valid::ValidationFlags, - bounds_check_policies: naga::back::BoundsCheckPolicies, + bounds_check_policies: naga::proc::BoundsCheckPolicies, entry_point: Option, keep_coordinate_space: bool, spv_block_ctx_dump_prefix: Option, diff --git a/src/back/mod.rs b/src/back/mod.rs index d2ebce38b4..571289a79b 100644 --- a/src/back/mod.rs +++ b/src/back/mod.rs @@ -123,117 +123,6 @@ impl<'a> FunctionCtx<'_> { } } -/// 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 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 `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. - /// - /// (This does not necessarily mean clamping. For example, interpreting the - /// index as unsigned and taking the minimum with the largest valid index - /// would also be a valid implementation. That would map negative indices to - /// the last element, not the first.) - Restrict, - - /// Out-of-bounds reads return zero, and writes have no effect. - ReadZeroSkipWrite, - - /// 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. - Unchecked, -} - -#[derive(Clone, Copy, Debug, Default)] -/// Policies for injecting bounds checks during code generation. -/// -/// For SPIR-V generation, see [`spv::Options::bounds_check_policies`]. -pub struct BoundsCheckPolicies { - /// How should the generated code handle array, vector, or matrix indices - /// that are out of range? - pub index: BoundsCheckPolicy, - - /// How should the generated code handle array, vector, or matrix indices - /// that are out of range, when those values live in a [`GlobalVariable`] in - /// the [`Storage`] or [`Uniform`] storage classes? - /// - /// Some graphics hardware provides "robust buffer access", a feature that - /// ensures that using a pointer cannot access memory outside the 'buffer' - /// that it was derived from. In Naga terms, this means that the hardware - /// ensures that pointers computed by applying [`Access`] and - /// [`AccessIndex`] expressions to a [`GlobalVariable`] whose [`class`] is - /// [`Storage`] or [`Uniform`] will never read or write memory outside that - /// global variable. - /// - /// When hardware offers such a feature, it is probably undesirable to have - /// Naga inject bounds checking code for such accesses, since the hardware - /// can probably provide the same protection more efficiently. However, - /// bounds checks are still needed on accesses to indexable values that do - /// not live in buffers, like local variables. - /// - /// So, this option provides a separate policy that applies only to accesses - /// to storage and uniform globals. When depending on hardware bounds - /// checking, this policy can be `Unchecked` to avoid unnecessary overhead. - /// - /// When special hardware support is not available, this should probably be - /// the same as `index_bounds_check_policy`. - /// - /// [`GlobalVariable`]: crate::GlobalVariable - /// [`class`]: crate::GlobalVariable::class - /// [`Restrict`]: crate::back::BoundsCheckPolicy::Restrict - /// [`ReadZeroSkipWrite`]: crate::back::BoundsCheckPolicy::ReadZeroSkipWrite - /// [`Access`]: crate::Expression::Access - /// [`AccessIndex`]: crate::Expression::AccessIndex - /// [`Storage`]: crate::StorageClass::Storage - /// [`Uniform`]: crate::StorageClass::Uniform - pub buffer: BoundsCheckPolicy, - - /// How should the generated code handle image texel references that are out - /// of range? - /// - /// This controls the behavior of [`ImageLoad`] expressions and - /// [`ImageStore`] statements when a coordinate, texture array index, level - /// of detail, or multisampled sample number is out of range. - /// - /// [`ImageLoad`]: crate::Expression::ImageLoad - /// [`ImageStore`]: crate::Statement::ImageStore - pub image: BoundsCheckPolicy, -} - -/// The default `BoundsCheckPolicy` is `Unchecked`. -impl Default for BoundsCheckPolicy { - fn default() -> Self { - BoundsCheckPolicy::Unchecked - } -} - impl crate::Expression { /// Returns the ref count, upon reaching which this expression /// should be considered for baking. diff --git a/src/back/spv/image.rs b/src/back/spv/image.rs index 5966ee0ef7..442045ed2a 100644 --- a/src/back/spv/image.rs +++ b/src/back/spv/image.rs @@ -739,7 +739,7 @@ impl<'w> BlockContext<'w> { // Perform the access, according to the bounds check policy. let access_id = match self.writer.bounds_check_policies.image { - crate::back::BoundsCheckPolicy::Restrict => { + crate::proc::BoundsCheckPolicy::Restrict => { let (coords, level_id, sample_id) = self.write_restricted_coordinates( image_id, coordinates, @@ -749,7 +749,7 @@ impl<'w> BlockContext<'w> { )?; access.generate(&mut self.writer.id_gen, coords, level_id, sample_id, block) } - crate::back::BoundsCheckPolicy::ReadZeroSkipWrite => self + crate::proc::BoundsCheckPolicy::ReadZeroSkipWrite => self .write_conditional_image_access( image_id, coordinates, @@ -758,7 +758,7 @@ impl<'w> BlockContext<'w> { block, &access, )?, - crate::back::BoundsCheckPolicy::Unchecked => access.generate( + crate::proc::BoundsCheckPolicy::Unchecked => access.generate( &mut self.writer.id_gen, coordinates.value_id, level_id, @@ -1131,12 +1131,12 @@ impl<'w> BlockContext<'w> { let write = Store { image_id, value_id }; match self.writer.bounds_check_policies.image { - crate::back::BoundsCheckPolicy::Restrict => { + crate::proc::BoundsCheckPolicy::Restrict => { let (coords, _, _) = self.write_restricted_coordinates(image_id, coordinates, None, None, block)?; write.generate(&mut self.writer.id_gen, coords, None, None, block); } - crate::back::BoundsCheckPolicy::ReadZeroSkipWrite => { + crate::proc::BoundsCheckPolicy::ReadZeroSkipWrite => { self.write_conditional_image_access( image_id, coordinates, @@ -1146,7 +1146,7 @@ impl<'w> BlockContext<'w> { &write, )?; } - crate::back::BoundsCheckPolicy::Unchecked => { + crate::proc::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 30ac58472f..bd66197626 100644 --- a/src/back/spv/index.rs +++ b/src/back/spv/index.rs @@ -1,7 +1,7 @@ //! Bounds-checking for SPIR-V output. use super::{selection::Selection, Block, BlockContext, Error, IdGenerator, Instruction, Word}; -use crate::{arena::Handle, back::BoundsCheckPolicy}; +use crate::{arena::Handle, proc::BoundsCheckPolicy}; /// The results of emitting code for a left-hand-side expression. /// @@ -342,22 +342,11 @@ impl<'w> BlockContext<'w> { index: Handle, block: &mut Block, ) -> Result { - // Should this access be covered by `index_bounds_check_policy` or - // `buffer_bounds_check_policy`? - let is_buffer = match *self.fun_info[base].ty.inner_with(&self.ir_module.types) { - crate::TypeInner::Pointer { class, .. } - | crate::TypeInner::ValuePointer { class, .. } => match class { - crate::StorageClass::Storage { access: _ } | crate::StorageClass::Uniform => true, - _ => false, - }, - _ => false, - }; - - let policy = if is_buffer { - self.writer.bounds_check_policies.buffer - } else { - self.writer.bounds_check_policies.index - }; + let policy = self.writer.bounds_check_policies.choose_policy( + base, + &self.ir_module.types, + self.fun_info, + ); Ok(match policy { BoundsCheckPolicy::Restrict => self.write_restricted_index(base, index, block)?, diff --git a/src/back/spv/mod.rs b/src/back/spv/mod.rs index 4eb75706b0..d341e8df2d 100644 --- a/src/back/spv/mod.rs +++ b/src/back/spv/mod.rs @@ -13,7 +13,8 @@ mod writer; pub use spirv::Capability; -use crate::{arena::Handle, back::BoundsCheckPolicies, proc::TypeResolution}; +use crate::arena::Handle; +use crate::proc::{BoundsCheckPolicies, TypeResolution}; use spirv::Word; use std::ops; @@ -604,7 +605,7 @@ impl Default for Options { lang_version: (1, 0), flags, capabilities: None, - bounds_check_policies: super::BoundsCheckPolicies::default(), + bounds_check_policies: crate::proc::BoundsCheckPolicies::default(), } } } diff --git a/src/proc/index.rs b/src/proc/index.rs index b6dd1e4b56..080be03cb0 100644 --- a/src/proc/index.rs +++ b/src/proc/index.rs @@ -1,6 +1,142 @@ //! Definitions for index bounds checking. use super::ProcError; +use crate::valid; +use crate::{Handle, UniqueArena}; + +/// 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 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 `Unchecked`, 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. + /// + /// (This does not necessarily mean clamping. For example, interpreting the + /// index as unsigned and taking the minimum with the largest valid index + /// would also be a valid implementation. That would map negative indices to + /// the last element, not the first.) + Restrict, + + /// Out-of-bounds reads return zero, and writes have no effect. + ReadZeroSkipWrite, + + /// 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. + Unchecked, +} + +#[derive(Clone, Copy, Debug, Default)] +/// Policies for injecting bounds checks during code generation. +pub struct BoundsCheckPolicies { + /// How should the generated code handle array, vector, or matrix indices + /// that are out of range? + pub index: BoundsCheckPolicy, + + /// How should the generated code handle array, vector, or matrix indices + /// that are out of range, when those values live in a [`GlobalVariable`] in + /// the [`Storage`] or [`Uniform`] storage classes? + /// + /// Some graphics hardware provides "robust buffer access", a feature that + /// ensures that using a pointer cannot access memory outside the 'buffer' + /// that it was derived from. In Naga terms, this means that the hardware + /// ensures that pointers computed by applying [`Access`] and + /// [`AccessIndex`] expressions to a [`GlobalVariable`] whose [`class`] is + /// [`Storage`] or [`Uniform`] will never read or write memory outside that + /// global variable. + /// + /// When hardware offers such a feature, it is probably undesirable to have + /// Naga inject bounds checking code for such accesses, since the hardware + /// can probably provide the same protection more efficiently. However, + /// bounds checks are still needed on accesses to indexable values that do + /// not live in buffers, like local variables. + /// + /// So, this option provides a separate policy that applies only to accesses + /// to storage and uniform globals. When depending on hardware bounds + /// checking, this policy can be `Unchecked` to avoid unnecessary overhead. + /// + /// When special hardware support is not available, this should probably be + /// the same as `index_bounds_check_policy`. + /// + /// [`GlobalVariable`]: crate::GlobalVariable + /// [`class`]: crate::GlobalVariable::class + /// [`Restrict`]: crate::back::BoundsCheckPolicy::Restrict + /// [`ReadZeroSkipWrite`]: crate::back::BoundsCheckPolicy::ReadZeroSkipWrite + /// [`Access`]: crate::Expression::Access + /// [`AccessIndex`]: crate::Expression::AccessIndex + /// [`Storage`]: crate::StorageClass::Storage + /// [`Uniform`]: crate::StorageClass::Uniform + pub buffer: BoundsCheckPolicy, + + /// How should the generated code handle image texel references that are out + /// of range? + /// + /// This controls the behavior of [`ImageLoad`] expressions and + /// [`ImageStore`] statements when a coordinate, texture array index, level + /// of detail, or multisampled sample number is out of range. + /// + /// [`ImageLoad`]: crate::Expression::ImageLoad + /// [`ImageStore`]: crate::Statement::ImageStore + pub image: BoundsCheckPolicy, +} + +/// The default `BoundsCheckPolicy` is `Unchecked`. +impl Default for BoundsCheckPolicy { + fn default() -> Self { + BoundsCheckPolicy::Unchecked + } +} + +impl BoundsCheckPolicies { + /// Determine which policy applies to a load or store of `pointer`. + /// + /// See the documentation for [`BoundsCheckPolicy`] for details about + /// when each policy applies. + pub fn choose_policy( + &self, + pointer: Handle, + types: &UniqueArena, + info: &valid::FunctionInfo, + ) -> BoundsCheckPolicy { + let is_buffer = match info[pointer].ty.inner_with(types).pointer_class() { + Some(crate::StorageClass::Storage { access: _ }) + | Some(crate::StorageClass::Uniform) => true, + _ => false, + }; + + if is_buffer { + self.buffer + } else { + self.index + } + } +} impl crate::TypeInner { /// Return the length of a subscriptable type. diff --git a/src/proc/mod.rs b/src/proc/mod.rs index 4c906f6eee..4c40d845d1 100644 --- a/src/proc/mod.rs +++ b/src/proc/mod.rs @@ -8,7 +8,7 @@ mod typifier; use std::cmp::PartialEq; -pub use index::IndexableLength; +pub use index::{BoundsCheckPolicies, BoundsCheckPolicy, IndexableLength}; pub use layouter::{Alignment, InvalidBaseType, Layouter, TypeLayout}; pub use namer::{EntryPointIndex, NameKey, Namer}; pub use terminator::ensure_block_returns; diff --git a/tests/snapshots.rs b/tests/snapshots.rs index 159e36c57d..eaae4f795d 100644 --- a/tests/snapshots.rs +++ b/tests/snapshots.rs @@ -30,7 +30,7 @@ impl Default for BoundsCheckPolicyArg { } } -impl From for naga::back::BoundsCheckPolicy { +impl From for naga::proc::BoundsCheckPolicy { fn from(arg: BoundsCheckPolicyArg) -> Self { match arg { BoundsCheckPolicyArg::Restrict => Self::Restrict, @@ -220,7 +220,7 @@ fn write_output_spv( Some(params.spv.capabilities.clone()) }, - bounds_check_policies: naga::back::BoundsCheckPolicies { + bounds_check_policies: naga::proc::BoundsCheckPolicies { index: params.index_bounds_check_policy.into(), buffer: params.buffer_bounds_check_policy.into(), image: params.image_bounds_check_policy.into(),