entry: apply NonWritable to read-only StorageBuffers.

CHANGELOG.md

@@ -29,6 +29,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed 🛠
+- [PR#1011](https://github.com/EmbarkStudios/rust-gpu/pull/1011) made `NonWritable` all read-only storage buffers (i.e. those typed `&T`, where `T` doesn't have interior mutability)
+
 ### Fixed 🩹
 - [PR#1009](https://github.com/EmbarkStudios/rust-gpu/pull/1009) fixed [#1008](https://github.com/EmbarkStudios/rust-gpu/issues/1008) by reinstating mutability checks for entry-point parameters pointing into read-only storage classes (e.g. `#[spirv(uniform)] x: &mut u32` is now again an error)
 - [PR#995](https://github.com/EmbarkStudios/rust-gpu/pull/995) fixed [#994](https://github.com/EmbarkStudios/rust-gpu/issues/994) by using `OpAtomicFAddEXT` instead of `OpAtomicFMaxEXT` in `atomic_f_add`.

crates/rustc_codegen_spirv/src/codegen_cx/entry.rs

@@ -41,6 +41,13 @@ struct EntryParamDeducedFromRustRefOrValue<'tcx> {
     /// either deduced from the type (e.g. opaque handles use `UniformConstant`),
     /// provided via `#[spirv(...)]` attributes, or an `Input`/`Output` default.
     storage_class: StorageClass,
+
+    /// Whether this entry-point parameter doesn't allow writes to the underlying
+    /// shader interface variable (i.e. is by-value, or `&T` where `T: Freeze`).
+    ///
+    /// For some storage classes, this can be mapped to `NonWritable` decorations
+    /// (only `StorageBuffer` for now, with few others, if any, plausible at all).
+    read_only: bool,
 }
 
 impl<'tcx> CodegenCx<'tcx> {
@@ -334,9 +341,10 @@ impl<'tcx> CodegenCx<'tcx> {
             });
 
         // Validate reference mutability against the *final* storage class.
+        let read_only = effective_mutbl == hir::Mutability::Not;
         if is_ref {
-            // FIXME(eddyb) booleans make the condition a bit more readable.
-            let ref_is_read_only = effective_mutbl == hir::Mutability::Not;
+            // FIXME(eddyb) named booleans make uses a bit more readable.
+            let ref_is_read_only = read_only;
             let storage_class_requires_read_only =
                 expected_mutbl_for(storage_class) == hir::Mutability::Not;
             if !ref_is_read_only && storage_class_requires_read_only {
@@ -378,6 +386,7 @@ impl<'tcx> CodegenCx<'tcx> {
         EntryParamDeducedFromRustRefOrValue {
             value_layout,
             storage_class,
+            read_only,
         }
     }
 
@@ -400,9 +409,21 @@ impl<'tcx> CodegenCx<'tcx> {
         let EntryParamDeducedFromRustRefOrValue {
             value_layout,
             storage_class,
+            read_only,
         } = self.entry_param_deduce_from_rust_ref_or_value(entry_arg_abi.layout, hir_param, &attrs);
         let value_spirv_type = value_layout.spirv_type(hir_param.ty_span, self);
 
+        // Emit decorations deduced from the reference/value Rust type.
+        if read_only {
+            // NOTE(eddyb) it appears only `StorageBuffer`s simultaneously:
+            // - allow `NonWritable` decorations on shader interface variables
+            // - default to writable (i.e. the decoration actually has an effect)
+            if storage_class == StorageClass::StorageBuffer {
+                self.emit_global()
+                    .decorate(var, Decoration::NonWritable, []);
+            }
+        }
+
         // Certain storage classes require an `OpTypeStruct` decorated with `Block`,
         // which we represent with `SpirvType::InterfaceBlock` (see its doc comment).
         // This "interface block" construct is also required for "runtime arrays".

tests/ui/dis/non-writable-storage_buffer.not_spirt.rs

@@ -0,0 +1,33 @@
+// HACK(eddyb) duplicate of non-writable-storage_buffer.spirt.rs because only-/ignore- do not work with revisions.
+// only-not_spirt
+#![crate_name = "non_writable_storage_buffer"]
+
+// Tests that only `&T` (where `T: Freeze`) storage buffers get `NonWritable`.
+
+// build-pass
+// compile-flags: -C llvm-args=--disassemble-globals
+// normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> ""
+// normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> ""
+// normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple"
+
+// FIXME(eddyb) this should use revisions to track both the `vulkan1.2` output
+// and the pre-`vulkan1.2` output, but per-revisions `{only,ignore}-*` directives
+// are not supported in `compiletest-rs`.
+// ignore-vulkan1.2
+
+use spirv_std::spirv;
+
+#[spirv(fragment)]
+pub fn main(
+    #[spirv(storage_buffer, descriptor_set = 0, binding = 0)] buf_imm: &u32,
+    #[spirv(storage_buffer, descriptor_set = 0, binding = 1)] buf_mut: &mut u32,
+    // FIXME(eddyb) use `AtomicU32` when methods on that work.
+    #[spirv(storage_buffer, descriptor_set = 0, binding = 2)]
+    buf_interior_mut: &core::cell::UnsafeCell<u32>,
+) {
+    let x = *buf_imm;
+    *buf_mut = x;
+    unsafe {
+        *buf_interior_mut.get() = x;
+    }
+}

tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr

@@ -0,0 +1,34 @@
+OpCapability Float64
+OpCapability Int16
+OpCapability Int64
+OpCapability Int8
+OpCapability ShaderClockKHR
+OpCapability Shader
+OpExtension "SPV_KHR_shader_clock"
+OpMemoryModel Logical Simple
+OpEntryPoint Fragment %1 "main"
+OpExecutionMode %1 OriginUpperLeft
+%2 = OpString "$OPSTRING_FILENAME/non-writable-storage_buffer.not_spirt.rs"
+%3 = OpString "$OPSTRING_FILENAME/cell.rs"
+OpName %4 "buf_imm"
+OpName %5 "buf_mut"
+OpName %6 "buf_interior_mut"
+OpDecorate %4 NonWritable
+OpDecorate %7 Block
+OpMemberDecorate %7 0 Offset 0
+OpDecorate %4 DescriptorSet 0
+OpDecorate %4 Binding 0
+OpDecorate %5 DescriptorSet 0
+OpDecorate %5 Binding 1
+OpDecorate %6 DescriptorSet 0
+OpDecorate %6 Binding 2
+%8 = OpTypeInt 32 0
+%9 = OpTypePointer StorageBuffer %8
+%10 = OpTypeVoid
+%11 = OpTypeFunction %10
+%7 = OpTypeStruct %8
+%12 = OpTypePointer StorageBuffer %7
+%13 = OpConstant  %8  0
+%4 = OpVariable  %12  StorageBuffer
+%5 = OpVariable  %12  StorageBuffer
+%6 = OpVariable  %12  StorageBuffer

tests/ui/dis/non-writable-storage_buffer.spirt.rs

@@ -0,0 +1,33 @@
+// HACK(eddyb) duplicate of non-writable-storage_buffer.not_spirt.rs because only-/ignore- do not work with revisions.
+// only-spirt
+#![crate_name = "non_writable_storage_buffer"]
+
+// Tests that only `&T` (where `T: Freeze`) storage buffers get `NonWritable`.
+
+// build-pass
+// compile-flags: -C llvm-args=--disassemble-globals
+// normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> ""
+// normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> ""
+// normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple"
+
+// FIXME(eddyb) this should use revisions to track both the `vulkan1.2` output
+// and the pre-`vulkan1.2` output, but per-revisions `{only,ignore}-*` directives
+// are not supported in `compiletest-rs`.
+// ignore-vulkan1.2
+
+use spirv_std::spirv;
+
+#[spirv(fragment)]
+pub fn main(
+    #[spirv(storage_buffer, descriptor_set = 0, binding = 0)] buf_imm: &u32,
+    #[spirv(storage_buffer, descriptor_set = 0, binding = 1)] buf_mut: &mut u32,
+    // FIXME(eddyb) use `AtomicU32` when methods on that work.
+    #[spirv(storage_buffer, descriptor_set = 0, binding = 2)]
+    buf_interior_mut: &core::cell::UnsafeCell<u32>,
+) {
+    let x = *buf_imm;
+    *buf_mut = x;
+    unsafe {
+        *buf_interior_mut.get() = x;
+    }
+}

tests/ui/dis/non-writable-storage_buffer.spirt.stderr

@@ -0,0 +1,33 @@
+OpCapability Shader
+OpCapability Float64
+OpCapability Int64
+OpCapability Int16
+OpCapability Int8
+OpCapability ShaderClockKHR
+OpExtension "SPV_KHR_shader_clock"
+OpMemoryModel Logical Simple
+OpEntryPoint Fragment %1 "main"
+OpExecutionMode %1 OriginUpperLeft
+%2 = OpString "$OPSTRING_FILENAME/non-writable-storage_buffer.spirt.rs"
+OpName %3 "buf_imm"
+OpName %4 "buf_mut"
+OpName %5 "buf_interior_mut"
+OpDecorate %6 Block
+OpMemberDecorate %6 0 Offset 0
+OpDecorate %3 NonWritable
+OpDecorate %3 Binding 0
+OpDecorate %3 DescriptorSet 0
+OpDecorate %4 Binding 1
+OpDecorate %4 DescriptorSet 0
+OpDecorate %5 Binding 2
+OpDecorate %5 DescriptorSet 0
+%7 = OpTypeVoid
+%8 = OpTypeFunction %7
+%9 = OpTypeInt 32 0
+%10 = OpTypePointer StorageBuffer %9
+%6 = OpTypeStruct %9
+%11 = OpTypePointer StorageBuffer %6
+%3 = OpVariable  %11  StorageBuffer
+%12 = OpConstant  %9  0
+%4 = OpVariable  %11  StorageBuffer
+%5 = OpVariable  %11  StorageBuffer