From d7280c3eb0341906a24084595b1a44aed24b3eb7 Mon Sep 17 00:00:00 2001
From: jsmall-nvidia <jsmall@nvidia.com>
Date: Wed, 13 Feb 2019 14:49:02 -0500
Subject: Output readonly for suitable glsl buffers (#845)

* Output readonly on buffers for glsl if resource is readonly.
Didn't add to emitGLSLParameterGroup because the cases there seem to to either be implicitly read only, or allow write.

* * Improve comments around use of 'readonly' on glsl output
* Use readonly with shaderRecord

* Add comment pointing out shader record can be rw on vk, so might require changes in the future.
---
 source/slang/emit.cpp                           | 56 +++++++++++++++++++++++--
 tests/cross-compile/array-of-buffers.slang.glsl |  4 +-
 tests/vkray/closesthit.slang.glsl               |  4 +-
 3 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/source/slang/emit.cpp b/source/slang/emit.cpp
index eeb23a6e5..c0e5e4296 100644
--- a/source/slang/emit.cpp
+++ b/source/slang/emit.cpp
@@ -5857,21 +5857,33 @@ struct EmitVisitor
             typeLayout = parameterGroupTypeLayout->elementVarLayout->typeLayout;
         }
 
+        /*
+        With resources backed by 'buffer' on glsl, we want to output 'readonly' if that is a good match
+        for the underlying type. If uniform it's implicit it's readonly
+
+        Here this only happens with isShaderRecord which is a 'constant buffer' (ie implicitly readonly)
+        or IRGLSLShaderStorageBufferType which is read write.
+        */
+
         emitGLSLLayoutQualifier(LayoutResourceKind::DescriptorTableSlot, &containerChain);
         emitGLSLLayoutQualifier(LayoutResourceKind::PushConstantBuffer, &containerChain);
         bool isShaderRecord = emitGLSLLayoutQualifier(LayoutResourceKind::ShaderRecord, &containerChain);
 
         if( isShaderRecord )
         {
-            emit("buffer ");
+            // TODO: A shader record in vk can be potentially read write. Currently slang does't support write access
+            // so for now we will assume readonly
+            emit("readonly buffer ");
         }
         else if(as<IRGLSLShaderStorageBufferType>(type))
         {
+            // Is writable 
             emit("layout(std430) buffer ");
         }
         // TODO: what to do with HLSL `tbuffer` style buffers?
         else
         {
+            // uniform is implicitly read only
             emit("layout(std140) uniform ");
         }
 
@@ -5995,7 +6007,27 @@ struct EmitVisitor
             }
         }
         
-        emit(") buffer ");
+        emit(") ");
+
+        /*
+        If the output type is a buffer, and we can determine it is only readonly we can prefix before
+        buffer with 'readonly'
+
+        The actual structuredBufferType could be
+
+        HLSLStructuredBufferType                        - This is unambiguously read only
+        HLSLRWStructuredBufferType                      - Read write
+        HLSLRasterizerOrderedStructuredBufferType       - Allows read/write access
+        HLSLAppendStructuredBufferType                  - Write
+        HLSLConsumeStructuredBufferType                 - TODO (JS): Its possible that this can be readonly, but we currently don't support on GLSL
+        */
+
+        if (as<IRHLSLStructuredBufferType>(structuredBufferType))
+        {
+            emit("readonly ");
+        }
+
+        emit("buffer ");
     
         // Generate a dummy name for the block
         emit("_S");
@@ -6021,7 +6053,7 @@ struct EmitVisitor
     void emitIRByteAddressBuffer_GLSL(
         EmitContext*                    ctx,
         IRGlobalParam*                  varDecl,
-        IRByteAddressBufferTypeBase*    /* byteAddressBufferType */)
+        IRByteAddressBufferTypeBase*    byteAddressBufferType)
     {
         // TODO: A lot of this logic is copy-pasted from `emitIRStructuredBuffer_GLSL`.
         // It might be worthwhile to share the common code to avoid regressions sneaking
@@ -6052,7 +6084,23 @@ struct EmitVisitor
             }
         }
 
-        emit(") buffer ");
+        emit(") ");
+
+        /*
+        If the output type is a buffer, and we can determine it is only readonly we can prefix before
+        buffer with 'readonly'
+
+        HLSLByteAddressBufferType                   - This is unambiguously read only
+        HLSLRWByteAddressBufferType                 - Read write
+        HLSLRasterizerOrderedByteAddressBufferType  - Allows read/write access
+        */
+
+        if (as<IRHLSLByteAddressBufferType>(byteAddressBufferType))
+        {
+            emit("readonly ");
+        }
+
+        emit("buffer ");
 
         // Generate a dummy name for the block
         emit("_S");
diff --git a/tests/cross-compile/array-of-buffers.slang.glsl b/tests/cross-compile/array-of-buffers.slang.glsl
index c3f271fe6..3dfe31d8d 100644
--- a/tests/cross-compile/array-of-buffers.slang.glsl
+++ b/tests/cross-compile/array-of-buffers.slang.glsl
@@ -25,7 +25,7 @@ layout(std140) uniform _S2
 
 
 layout(std430, binding = 2)
-buffer _S3 {
+readonly buffer _S3 {
     S_0 _data[];
 } sb1_0[4];
 
@@ -35,7 +35,7 @@ buffer _S4 {
 } sb2_0[5];
 
 layout(std430, binding = 4)
-buffer _S5
+readonly buffer _S5
 {
     uint _data[];
 } bb_0[6];
diff --git a/tests/vkray/closesthit.slang.glsl b/tests/vkray/closesthit.slang.glsl
index 46c5ea636..d85e982fa 100644
--- a/tests/vkray/closesthit.slang.glsl
+++ b/tests/vkray/closesthit.slang.glsl
@@ -21,12 +21,12 @@ struct SLANG_ParameterGroup_ShaderRecord_0
 };
 
 layout(shaderRecordNV)
-buffer tmp_shaderrecord
+readonly buffer tmp_shaderrecord
 {
     SLANG_ParameterGroup_ShaderRecord_0 _data;
 } ShaderRecord_0;
 
-layout(std430, binding = 0) buffer tmp_colors
+layout(std430, binding = 0) readonly buffer tmp_colors
 {
     vec4 _data[];
 } colors_0;
-- 
cgit v1.2.3