From 1af922a607ac05737c24d8121d430ec5be8139f3 Mon Sep 17 00:00:00 2001
From: Rua <ruawhitepaw@gmail.com>
Date: Mon, 5 Apr 2021 12:53:18 +0200
Subject: [PATCH] Fix and refine the generation of `readonly` descriptor
 attribute (#1534)

* Vulkano-shaders: Improve handling of readonly descriptors

* Changelog

* Fixed image readonly detection for real this time
---
 CHANGELOG_VULKANO.md                   |  1 +
 vulkano-shaders/src/descriptor_sets.rs | 63 +++++++++++++++-----------
 2 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/CHANGELOG_VULKANO.md b/CHANGELOG_VULKANO.md
index 24f26d18..954d90cd 100644
--- a/CHANGELOG_VULKANO.md
+++ b/CHANGELOG_VULKANO.md
@@ -4,6 +4,7 @@
     Breaking changes should be listed first, before other changes, and should be preceded by - **Breaking**.
 -->
 - The deprecated `cause` trait function on Vulkano error types is replaced with `source`.
+- Vulkano-shaders: Fixed and refined the generation of the `readonly` descriptor attribute. It should now correctly mark uniforms and sampled images as read-only, but storage buffers and images only if explicitly marked as `readonly` in the shader.
 
 # Version 0.22.0 (2021-03-31)
 
diff --git a/vulkano-shaders/src/descriptor_sets.rs b/vulkano-shaders/src/descriptor_sets.rs
index 47b7c7cd..50a5cf53 100644
--- a/vulkano-shaders/src/descriptor_sets.rs
+++ b/vulkano-shaders/src/descriptor_sets.rs
@@ -16,6 +16,7 @@ use crate::parse::{Instruction, Spirv};
 use crate::{spirv_search, TypesMeta};
 use std::collections::HashSet;
 
+#[derive(Debug)]
 struct Descriptor {
     set: u32,
     binding: u32,
@@ -177,6 +178,10 @@ fn find_descriptors(doc: &Spirv, entrypoint_id: u32, interface: &[u32]) -> Vec<D
             .get_decoration_params(variable_id, Decoration::DecorationBinding)
             .unwrap()[0];
 
+        let nonwritable = doc
+            .get_decoration_params(variable_id, Decoration::DecorationNonWritable)
+            .is_some();
+
         // Find information about the kind of binding for this descriptor.
         let (desc_ty, readonly, array_count) =
             descriptor_infos(doc, pointed_ty, storage_class, false).expect(&format!(
@@ -188,7 +193,7 @@ fn find_descriptors(doc: &Spirv, entrypoint_id: u32, interface: &[u32]) -> Vec<D
             set,
             binding,
             array_count,
-            readonly,
+            readonly: nonwritable || readonly,
         });
     }
 
@@ -389,26 +394,23 @@ fn descriptor_infos(
                         decoration_block ^ decoration_buffer_block,
                         "Structs in shader interface are expected to be decorated with one of Block or BufferBlock"
                     );
-                    let is_ssbo = pointer_storage == StorageClass::StorageClassStorageBuffer;
 
-                    // Determine whether there's a NonWritable decoration.
-                    let readonly = {
-                        let mut readonly = vec![false; member_types.len()];
-                        for i in doc.instructions.iter() {
-                            if let Instruction::MemberDecorate { target_id, member, decoration, .. } = i {
-                                if *target_id == *result_id && *decoration == Decoration::DecorationNonWritable {
-                                    assert!(!readonly[*member as usize]);
-                                    readonly[*member as usize] = true;
-                                }
-                            }
-                        }
-                        readonly.iter().all(|x| *x)
-                    };
+                    // false -> VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER
+                    // true -> VK_DESCRIPTOR_TYPE_STORAGE_BUFFER
+                    let storage = pointer_storage == StorageClass::StorageClassStorageBuffer;
+
+                    // Determine whether all members have a NonWritable decoration.
+                    let nonwritable = (0..member_types.len() as u32).all(|i| {
+                        doc.get_member_decoration_params(pointed_ty, i, Decoration::DecorationNonWritable).is_some()
+                    });
+
+                    // Uniforms are never writable.
+                    let readonly = !storage || nonwritable;
 
                     let desc = quote! {
                         DescriptorDescTy::Buffer(DescriptorBufferDesc {
                             dynamic: None,
-                            storage: #is_ssbo,
+                            storage: #storage,
                         })
                     };
 
@@ -435,7 +437,7 @@ fn descriptor_infos(
 
                     match dim {
                         Dim::DimSubpassData => {
-                            // We are an input attachment.
+                            // VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT
                             assert!(
                                 !force_combined_image_sampled,
                                 "An OpTypeSampledImage can't point to \
@@ -459,25 +461,32 @@ fn descriptor_infos(
                                 }
                             };
 
-                            Some((desc, true, 1))
+                            Some((desc, true, 1)) // Never writable.
                         }
                         Dim::DimBuffer => {
-                            // We are a texel buffer.
-                            let not_sampled = !sampled;
+                            // false -> VK_DESCRIPTOR_TYPE_UNIFORM_TEXEL_BUFFER
+                            // true -> VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER
+                            let storage = !sampled;
                             let desc = quote! {
                                 DescriptorDescTy::TexelBuffer {
-                                    storage: #not_sampled,
+                                    storage: #storage,
                                     format: None, // TODO: specify format if known
                                 }
                             };
 
-                            Some((desc, true, 1))
+                            Some((desc, !storage, 1)) // Uniforms are never writable.
                         }
                         _ => {
-                            // We are a sampled or storage image.
-                            let ty = match force_combined_image_sampled {
-                                true => quote! { DescriptorDescTy::CombinedImageSampler },
-                                false => quote! { DescriptorDescTy::Image },
+                            let (ty, readonly) = match force_combined_image_sampled {
+                                // VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER
+                                // Never writable.
+                                true => (quote! { DescriptorDescTy::CombinedImageSampler }, true),
+                                false => {
+                                    // false -> VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE
+                                    // true -> VK_DESCRIPTOR_TYPE_STORAGE_IMAGE
+                                    let storage = !sampled;
+                                    (quote! { DescriptorDescTy::Image }, !storage) // Sampled images are never writable.
+                                },
                             };
                             let dim = match *dim {
                                 Dim::Dim1D => {
@@ -504,7 +513,7 @@ fn descriptor_infos(
                                 })
                             };
 
-                            Some((desc, true, 1))
+                            Some((desc, readonly, 1))
                         }
                     }
                 }