diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bddbb4542..551754b8d9 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs b/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs index 21b11098a7..461f35fac0 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs +++ b/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". diff --git a/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs b/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs new file mode 100644 index 0000000000..ba25af924f --- /dev/null +++ b/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, +) { + let x = *buf_imm; + *buf_mut = x; + unsafe { + *buf_interior_mut.get() = x; + } +} diff --git a/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr b/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr new file mode 100644 index 0000000000..d04f4bcd31 --- /dev/null +++ b/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 diff --git a/tests/ui/dis/non-writable-storage_buffer.spirt.rs b/tests/ui/dis/non-writable-storage_buffer.spirt.rs new file mode 100644 index 0000000000..5548788222 --- /dev/null +++ b/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, +) { + let x = *buf_imm; + *buf_mut = x; + unsafe { + *buf_interior_mut.get() = x; + } +} diff --git a/tests/ui/dis/non-writable-storage_buffer.spirt.stderr b/tests/ui/dis/non-writable-storage_buffer.spirt.stderr new file mode 100644 index 0000000000..426974272d --- /dev/null +++ b/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