Move BoundsCheckPolicy/Policies into proc, from back. (gfx-rs…

…#1537)

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<Self, Self::Err> {
-        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<String>,
     keep_coordinate_space: bool,
     spv_block_ctx_dump_prefix: Option<String>,

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.

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,

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<crate::Expression>,
         block: &mut Block,
     ) -> Result<BoundsCheckResult, Error> {
-        // 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)?,

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(),
         }
     }
 }

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<crate::Expression>,
+        types: &UniqueArena<crate::Type>,
+        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.