From 1b95e023e79357a85258e68f88fa20ed1a79fb30 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 24 Aug 2021 18:26:22 -0700 Subject: [PATCH] [spv-out] Support a separate bounds check policy for buffers. --- cli/src/main.rs | 13 +++ src/back/spv/index.rs | 25 ++++- src/back/spv/mod.rs | 38 ++++++++ src/back/spv/writer.rs | 2 + tests/in/bounds-check-zero.param.ron | 1 + tests/in/policy-mix.param.ron | 7 ++ tests/in/policy-mix.wgsl | 35 +++++++ tests/out/spv/policy-mix.spvasm | 137 +++++++++++++++++++++++++++ tests/snapshots.rs | 5 + 9 files changed, 261 insertions(+), 2 deletions(-) create mode 100644 tests/in/policy-mix.param.ron create mode 100644 tests/in/policy-mix.wgsl create mode 100644 tests/out/spv/policy-mix.spvasm diff --git a/cli/src/main.rs b/cli/src/main.rs index f0ddbbd47c..05d459ce09 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -17,6 +17,14 @@ struct Args { #[argh(option)] index_bounds_check_policy: Option, + /// what policy to use for index bounds checking for arrays, vectors, and + /// matrices, when they are stored in globals in the `storage` or `uniform` + /// storage classes. + /// + /// May be `Restrict`, `ReadZeroSkipWrite`, or `Unchecked` + #[argh(option)] + buffer_bounds_check_policy: Option, + /// what policy to use for texture bounds checking. /// /// May be `Restrict`, `ReadZeroSkipWrite`, or `Unchecked` @@ -114,6 +122,7 @@ impl FromStr for GlslProfileArg { struct Parameters { validation_flags: naga::valid::ValidationFlags, index_bounds_check_policy: naga::back::BoundsCheckPolicy, + buffer_bounds_check_policy: naga::back::BoundsCheckPolicy, image_bounds_check_policy: naga::back::BoundsCheckPolicy, entry_point: Option, spv_adjust_coordinate_space: bool, @@ -194,6 +203,9 @@ fn run() -> Result<(), Box> { if let Some(policy) = args.index_bounds_check_policy { params.index_bounds_check_policy = policy.0; } + if let Some(policy) = args.buffer_bounds_check_policy { + params.buffer_bounds_check_policy = policy.0; + } if let Some(policy) = args.image_bounds_check_policy { params.image_bounds_check_policy = policy.0; } @@ -318,6 +330,7 @@ fn run() -> Result<(), Box> { use naga::back::spv; params.spv.index_bounds_check_policy = params.index_bounds_check_policy; + params.spv.buffer_bounds_check_policy = params.buffer_bounds_check_policy; params.spv.image_bounds_check_policy = params.image_bounds_check_policy; let spv = spv::write_vec( diff --git a/src/back/spv/index.rs b/src/back/spv/index.rs index a6cf6bfb8f..53b1d48cb8 100644 --- a/src/back/spv/index.rs +++ b/src/back/spv/index.rs @@ -332,7 +332,11 @@ impl<'w> BlockContext<'w> { selection.finish(self, loaded_value) } - /// Emit code for bounds checks, per self.index_bounds_check_policy. + /// Emit code for bounds checks for an array, vector, or matrix access. + /// + /// This implements either `index_bounds_check_policy` or + /// `buffer_bounds_check_policy`, depending on the storage class of the + /// pointer being accessed. /// /// Return a `BoundsCheckResult` indicating how the index should be /// consumed. See that type's documentation for details. @@ -342,7 +346,24 @@ impl<'w> BlockContext<'w> { index: Handle, block: &mut Block, ) -> Result { - Ok(match self.writer.index_bounds_check_policy { + // 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.buffer_bounds_check_policy + } else { + self.writer.index_bounds_check_policy + }; + + Ok(match policy { BoundsCheckPolicy::Restrict => self.write_restricted_index(base, index, block)?, BoundsCheckPolicy::ReadZeroSkipWrite => { self.write_index_comparison(base, index, block)? diff --git a/src/back/spv/mod.rs b/src/back/spv/mod.rs index 9da70e0ce8..d57e53975c 100644 --- a/src/back/spv/mod.rs +++ b/src/back/spv/mod.rs @@ -419,6 +419,7 @@ pub struct Writer { flags: WriterFlags, index_bounds_check_policy: BoundsCheckPolicy, image_bounds_check_policy: BoundsCheckPolicy, + buffer_bounds_check_policy: BoundsCheckPolicy, void_type: Word, //TODO: convert most of these into vectors, addressable by handle indices lookup_type: crate::FastHashMap, @@ -463,6 +464,42 @@ pub struct Options { /// How should the generated code handle array, vector, or matrix indices /// that are out of range? pub index_bounds_check_policy: 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_bounds_check_policy: BoundsCheckPolicy, + /// How should the generated code handle image references that are out of /// range? pub image_bounds_check_policy: BoundsCheckPolicy, @@ -480,6 +517,7 @@ impl Default for Options { capabilities: None, index_bounds_check_policy: super::BoundsCheckPolicy::default(), image_bounds_check_policy: super::BoundsCheckPolicy::default(), + buffer_bounds_check_policy: super::BoundsCheckPolicy::default(), } } } diff --git a/src/back/spv/writer.rs b/src/back/spv/writer.rs index dac71b19f9..f845a7ff60 100644 --- a/src/back/spv/writer.rs +++ b/src/back/spv/writer.rs @@ -68,6 +68,7 @@ impl Writer { flags: options.flags, index_bounds_check_policy: options.index_bounds_check_policy, image_bounds_check_policy: options.image_bounds_check_policy, + buffer_bounds_check_policy: options.buffer_bounds_check_policy, void_type, lookup_type: crate::FastHashMap::default(), lookup_function: crate::FastHashMap::default(), @@ -105,6 +106,7 @@ impl Writer { flags: self.flags, index_bounds_check_policy: self.index_bounds_check_policy, image_bounds_check_policy: self.image_bounds_check_policy, + buffer_bounds_check_policy: self.buffer_bounds_check_policy, capabilities_available: take(&mut self.capabilities_available), // Initialized afresh: diff --git a/tests/in/bounds-check-zero.param.ron b/tests/in/bounds-check-zero.param.ron index 1e772dd177..10f3305b42 100644 --- a/tests/in/bounds-check-zero.param.ron +++ b/tests/in/bounds-check-zero.param.ron @@ -1,4 +1,5 @@ ( index_bounds_check_policy: ReadZeroSkipWrite, + buffer_bounds_check_policy: ReadZeroSkipWrite, spv_version: (1, 1), ) diff --git a/tests/in/policy-mix.param.ron b/tests/in/policy-mix.param.ron new file mode 100644 index 0000000000..e3f7a182cf --- /dev/null +++ b/tests/in/policy-mix.param.ron @@ -0,0 +1,7 @@ +( + index_bounds_check_policy: Restrict, + buffer_bounds_check_policy: Unchecked, + image_bounds_check_policy: ReadZeroSkipWrite, + spv_version: (1, 1), + spv_debug: true, +) diff --git a/tests/in/policy-mix.wgsl b/tests/in/policy-mix.wgsl new file mode 100644 index 0000000000..e9b55709c9 --- /dev/null +++ b/tests/in/policy-mix.wgsl @@ -0,0 +1,35 @@ +// Tests that the index, buffer, and texture bounds checks policies are +// implemented separately. + +// Storage and Uniform storage classes +[[block]] +struct InStorage { + a: array, 10>; +}; +[[group(0), binding(0)]] var in_storage: InStorage; + +[[block]] +struct InUniform { + a: array, 20>; +}; +[[group(0), binding(1)]] var in_uniform: InUniform; + +// Textures automatically land in the `handle` storage class. +[[group(0), binding(2)]] var image_2d_array: texture_2d_array; + +// None of the above. +var in_workgroup: array; +var in_private: array; + +fn mock_function(c: vec2, i: i32, l: i32) -> vec4 { + var in_function: array, 2> = + array, 2>(vec4(0.707, 0.0, 0.0, 1.0), + vec4(0.0, 0.707, 0.0, 1.0)); + + return (in_storage.a[i] + + in_uniform.a[i] + + textureLoad(image_2d_array, c, i, l) + + in_workgroup[i] + + in_private[i] + + in_function[i]); +} diff --git a/tests/out/spv/policy-mix.spvasm b/tests/out/spv/policy-mix.spvasm new file mode 100644 index 0000000000..466cc8585d --- /dev/null +++ b/tests/out/spv/policy-mix.spvasm @@ -0,0 +1,137 @@ +; SPIR-V +; Version: 1.1 +; Generator: rspirv +; Bound: 93 +OpCapability Shader +OpCapability ImageQuery +OpCapability Linkage +OpExtension "SPV_KHR_storage_buffer_storage_class" +%1 = OpExtInstImport "GLSL.std.450" +OpMemoryModel Logical GLSL450 +OpSource GLSL 450 +OpName %15 "InStorage" +OpMemberName %15 0 "a" +OpName %17 "InUniform" +OpMemberName %17 0 "a" +OpName %23 "in_storage" +OpName %25 "in_uniform" +OpName %27 "image_2d_array" +OpName %29 "in_workgroup" +OpName %31 "in_private" +OpName %33 "in_function" +OpName %39 "mock_function" +OpDecorate %14 ArrayStride 16 +OpDecorate %15 Block +OpMemberDecorate %15 0 Offset 0 +OpDecorate %16 ArrayStride 16 +OpDecorate %17 Block +OpMemberDecorate %17 0 Offset 0 +OpDecorate %19 ArrayStride 4 +OpDecorate %20 ArrayStride 4 +OpDecorate %22 ArrayStride 16 +OpDecorate %23 NonWritable +OpDecorate %23 DescriptorSet 0 +OpDecorate %23 Binding 0 +OpDecorate %25 DescriptorSet 0 +OpDecorate %25 Binding 1 +OpDecorate %27 DescriptorSet 0 +OpDecorate %27 Binding 2 +%2 = OpTypeVoid +%4 = OpTypeInt 32 1 +%3 = OpConstant %4 10 +%5 = OpConstant %4 20 +%6 = OpConstant %4 30 +%7 = OpConstant %4 40 +%8 = OpConstant %4 2 +%10 = OpTypeFloat 32 +%9 = OpConstant %10 0.707 +%11 = OpConstant %10 0.0 +%12 = OpConstant %10 1.0 +%13 = OpTypeVector %10 4 +%14 = OpTypeArray %13 %3 +%15 = OpTypeStruct %14 +%16 = OpTypeArray %13 %5 +%17 = OpTypeStruct %16 +%18 = OpTypeImage %10 2D 0 1 0 1 Unknown +%19 = OpTypeArray %10 %6 +%20 = OpTypeArray %10 %7 +%21 = OpTypeVector %4 2 +%22 = OpTypeArray %13 %8 +%24 = OpTypePointer StorageBuffer %15 +%23 = OpVariable %24 StorageBuffer +%26 = OpTypePointer Uniform %17 +%25 = OpVariable %26 Uniform +%28 = OpTypePointer UniformConstant %18 +%27 = OpVariable %28 UniformConstant +%30 = OpTypePointer Workgroup %19 +%29 = OpVariable %30 Workgroup +%32 = OpTypePointer Private %20 +%31 = OpVariable %32 Private +%34 = OpTypePointer Function %22 +%40 = OpTypeFunction %13 %21 %4 %4 +%46 = OpTypePointer StorageBuffer %14 +%47 = OpTypePointer StorageBuffer %13 +%49 = OpTypeInt 32 0 +%48 = OpConstant %49 0 +%52 = OpTypePointer Uniform %16 +%53 = OpTypePointer Uniform %13 +%57 = OpTypeVector %4 3 +%59 = OpTypeBool +%60 = OpConstantNull %13 +%66 = OpTypeVector %59 3 +%73 = OpTypePointer Workgroup %10 +%74 = OpConstant %49 29 +%80 = OpTypePointer Private %10 +%81 = OpConstant %49 39 +%87 = OpTypePointer Function %13 +%88 = OpConstant %49 1 +%39 = OpFunction %13 None %40 +%36 = OpFunctionParameter %21 +%37 = OpFunctionParameter %4 +%38 = OpFunctionParameter %4 +%35 = OpLabel +%33 = OpVariable %34 Function +%41 = OpLoad %18 %27 +OpBranch %42 +%42 = OpLabel +%43 = OpCompositeConstruct %13 %9 %11 %11 %12 +%44 = OpCompositeConstruct %13 %11 %9 %11 %12 +%45 = OpCompositeConstruct %22 %43 %44 +OpStore %33 %45 +%50 = OpAccessChain %47 %23 %48 %37 +%51 = OpLoad %13 %50 +%54 = OpAccessChain %53 %25 %48 %37 +%55 = OpLoad %13 %54 +%56 = OpFAdd %13 %51 %55 +%58 = OpCompositeConstruct %57 %36 %37 +%61 = OpImageQueryLevels %4 %41 +%62 = OpULessThan %59 %38 %61 +OpSelectionMerge %63 None +OpBranchConditional %62 %64 %63 +%64 = OpLabel +%65 = OpImageQuerySizeLod %57 %41 %38 +%67 = OpULessThan %66 %58 %65 +%68 = OpAll %59 %67 +OpBranchConditional %68 %69 %63 +%69 = OpLabel +%70 = OpImageFetch %13 %41 %58 Lod %38 +OpBranch %63 +%63 = OpLabel +%71 = OpPhi %13 %60 %42 %60 %64 %70 %69 +%72 = OpFAdd %13 %56 %71 +%75 = OpExtInst %49 %1 UMin %37 %74 +%76 = OpAccessChain %73 %29 %75 +%77 = OpLoad %10 %76 +%78 = OpCompositeConstruct %13 %77 %77 %77 %77 +%79 = OpFAdd %13 %72 %78 +%82 = OpExtInst %49 %1 UMin %37 %81 +%83 = OpAccessChain %80 %31 %82 +%84 = OpLoad %10 %83 +%85 = OpCompositeConstruct %13 %84 %84 %84 %84 +%86 = OpFAdd %13 %79 %85 +%89 = OpExtInst %49 %1 UMin %37 %88 +%90 = OpAccessChain %87 %33 %89 +%91 = OpLoad %13 %90 +%92 = OpFAdd %13 %86 %91 +OpReturnValue %92 +OpFunctionEnd \ No newline at end of file diff --git a/tests/snapshots.rs b/tests/snapshots.rs index 5c44c7a4a1..6902520d9f 100644 --- a/tests/snapshots.rs +++ b/tests/snapshots.rs @@ -53,6 +53,9 @@ struct Parameters { index_bounds_check_policy: BoundsCheckPolicyArg, #[allow(dead_code)] #[serde(default)] + buffer_bounds_check_policy: BoundsCheckPolicyArg, + #[allow(dead_code)] + #[serde(default)] image_bounds_check_policy: BoundsCheckPolicyArg, #[cfg_attr(not(feature = "spv-out"), allow(dead_code))] @@ -187,6 +190,7 @@ fn write_output_spv( }, index_bounds_check_policy: params.index_bounds_check_policy.into(), + buffer_bounds_check_policy: params.buffer_bounds_check_policy.into(), image_bounds_check_policy: params.image_bounds_check_policy.into(), ..spv::Options::default() @@ -462,6 +466,7 @@ fn convert_wgsl() { ("bounds-check-zero", Targets::SPIRV), ("bounds-check-image-restrict", Targets::SPIRV), ("bounds-check-image-rzsw", Targets::SPIRV), + ("policy-mix", Targets::SPIRV), ( "texture-arg", Targets::SPIRV | Targets::METAL | Targets::GLSL | Targets::HLSL | Targets::WGSL,