Merge pull request #1287 from KhronosGroup/read-write-decoration-tweaks

Improve handling of readonly/writeonly flags in some cases

CMakeLists.txt

@@ -323,7 +323,7 @@ if (SPIRV_CROSS_STATIC)
 endif()
 
 set(spirv-cross-abi-major 0)
-set(spirv-cross-abi-minor 25)
+set(spirv-cross-abi-minor 27)
 set(spirv-cross-abi-patch 0)
 
 if (SPIRV_CROSS_SHARED)

main.cpp

@@ -527,6 +527,7 @@ struct CLIArguments
 	bool glsl_emit_ubo_as_plain_uniforms = false;
 	bool vulkan_glsl_disable_ext_samplerless_texture_functions = false;
 	bool emit_line_directives = false;
+	bool enable_storage_image_qualifier_deduction = true;
 	SmallVector<uint32_t> msl_discrete_descriptor_sets;
 	SmallVector<uint32_t> msl_device_argument_buffers;
 	SmallVector<pair<uint32_t, uint32_t>> msl_dynamic_buffers;
@@ -556,6 +557,7 @@ struct CLIArguments
 	bool hlsl = false;
 	bool hlsl_compat = false;
 	bool hlsl_support_nonzero_base = false;
+	bool hlsl_force_storage_buffer_as_uav = false;
 	HLSLBindingFlags hlsl_binding_flags = 0;
 	bool vulkan_semantics = false;
 	bool flatten_multidimensional_arrays = false;
@@ -594,6 +596,7 @@ static void print_help()
 	                "\t[--iterations iter]\n"
 	                "\t[--cpp]\n"
 	                "\t[--cpp-interface-name <name>]\n"
+	                "\t[--disable-storage-image-qualifier-deduction]\n"
 	                "\t[--glsl-emit-push-constant-as-ubo]\n"
 	                "\t[--glsl-emit-ubo-as-plain-uniforms]\n"
 	                "\t[--vulkan-glsl-disable-ext-samplerless-texture-functions]\n"
@@ -624,6 +627,7 @@ static void print_help()
 	                "\t[--hlsl-enable-compat]\n"
 	                "\t[--hlsl-support-nonzero-basevertex-baseinstance]\n"
 	                "\t[--hlsl-auto-binding (push, cbv, srv, uav, sampler, all)]\n"
+	                "\t[--hlsl-force-storage-buffer-as-uav]\n"
 	                "\t[--separate-shader-objects]\n"
 	                "\t[--pls-in format input-name]\n"
 	                "\t[--pls-out format output-name]\n"
@@ -942,6 +946,7 @@ static string compile_iteration(const CLIArguments &args, std::vector<uint32_t>
 	opts.emit_push_constant_as_uniform_buffer = args.glsl_emit_push_constant_as_ubo;
 	opts.emit_uniform_buffer_as_plain_uniforms = args.glsl_emit_ubo_as_plain_uniforms;
 	opts.emit_line_directives = args.emit_line_directives;
+	opts.enable_storage_image_qualifier_deduction = args.enable_storage_image_qualifier_deduction;
 	compiler->set_common_options(opts);
 
 	// Set HLSL specific options.
@@ -974,6 +979,7 @@ static string compile_iteration(const CLIArguments &args, std::vector<uint32_t>
 		}
 
 		hlsl_opts.support_nonzero_base_vertex_base_instance = args.hlsl_support_nonzero_base;
+		hlsl_opts.force_storage_buffer_as_uav = args.hlsl_force_storage_buffer_as_uav;
 		hlsl->set_hlsl_options(hlsl_opts);
 		hlsl->set_resource_binding_flags(args.hlsl_binding_flags);
 	}
@@ -1121,6 +1127,7 @@ static int main_inner(int argc, char *argv[])
 	cbs.add("--glsl-emit-ubo-as-plain-uniforms", [&args](CLIParser &) { args.glsl_emit_ubo_as_plain_uniforms = true; });
 	cbs.add("--vulkan-glsl-disable-ext-samplerless-texture-functions",
 	        [&args](CLIParser &) { args.vulkan_glsl_disable_ext_samplerless_texture_functions = true; });
+	cbs.add("--disable-storage-image-qualifier-deduction", [&args](CLIParser &) { args.enable_storage_image_qualifier_deduction = false; });
 	cbs.add("--msl", [&args](CLIParser &) { args.msl = true; });
 	cbs.add("--hlsl", [&args](CLIParser &) { args.hlsl = true; });
 	cbs.add("--hlsl-enable-compat", [&args](CLIParser &) { args.hlsl_compat = true; });
@@ -1129,6 +1136,9 @@ static int main_inner(int argc, char *argv[])
 	cbs.add("--hlsl-auto-binding", [&args](CLIParser &parser) {
 		args.hlsl_binding_flags |= hlsl_resource_type_to_flag(parser.next_string());
 	});
+	cbs.add("--hlsl-force-storage-buffer-as-uav", [&args](CLIParser &) {
+		args.hlsl_force_storage_buffer_as_uav = true;
+	});
 	cbs.add("--vulkan-semantics", [&args](CLIParser &) { args.vulkan_semantics = true; });
 	cbs.add("-V", [&args](CLIParser &) { args.vulkan_semantics = true; });
 	cbs.add("--flatten-multidimensional-arrays", [&args](CLIParser &) { args.flatten_multidimensional_arrays = true; });

reference/opt/shaders-hlsl/comp/access-chains.force-uav.comp

@@ -0,0 +1,23 @@
+static const uint3 gl_WorkGroupSize = uint3(1u, 1u, 1u);
+
+RWByteAddressBuffer wo : register(u1);
+RWByteAddressBuffer ro : register(u0);
+
+static uint3 gl_GlobalInvocationID;
+struct SPIRV_Cross_Input
+{
+    uint3 gl_GlobalInvocationID : SV_DispatchThreadID;
+};
+
+void comp_main()
+{
+    wo.Store4(gl_GlobalInvocationID.x * 64 + 272, asuint(asfloat(ro.Load4(gl_GlobalInvocationID.x * 64 + 160))));
+    wo.Store4(gl_GlobalInvocationID.x * 16 + 480, asuint(asfloat(ro.Load4(gl_GlobalInvocationID.x * 16 + 480))));
+}
+
+[numthreads(1, 1, 1)]
+void main(SPIRV_Cross_Input stage_input)
+{
+    gl_GlobalInvocationID = stage_input.gl_GlobalInvocationID;
+    comp_main();
+}

reference/opt/shaders/desktop-only/frag/image-size.frag

@@ -0,0 +1,12 @@
+#version 450
+
+layout(binding = 0, r32f) uniform readonly writeonly image2D uImage1;
+layout(binding = 1, r32f) uniform readonly writeonly image2D uImage2;
+
+layout(location = 0) out vec4 FragColor;
+
+void main()
+{
+    FragColor = vec4(vec2(imageSize(uImage1)), vec2(imageSize(uImage2)));
+}
+

reference/opt/shaders/desktop-only/frag/image-size.no-qualifier-deduction.frag

@@ -0,0 +1,12 @@
+#version 450
+
+layout(binding = 0, r32f) uniform image2D uImage1;
+layout(binding = 1, r32f) uniform image2D uImage2;
+
+layout(location = 0) out vec4 FragColor;
+
+void main()
+{
+    FragColor = vec4(vec2(imageSize(uImage1)), vec2(imageSize(uImage2)));
+}
+

reference/shaders-hlsl/comp/access-chains.force-uav.comp

@@ -0,0 +1,23 @@
+static const uint3 gl_WorkGroupSize = uint3(1u, 1u, 1u);
+
+RWByteAddressBuffer wo : register(u1);
+RWByteAddressBuffer ro : register(u0);
+
+static uint3 gl_GlobalInvocationID;
+struct SPIRV_Cross_Input
+{
+    uint3 gl_GlobalInvocationID : SV_DispatchThreadID;
+};
+
+void comp_main()
+{
+    wo.Store4(gl_GlobalInvocationID.x * 64 + 272, asuint(asfloat(ro.Load4(gl_GlobalInvocationID.x * 64 + 160))));
+    wo.Store4(gl_GlobalInvocationID.x * 16 + 480, asuint(asfloat(ro.Load4(gl_GlobalInvocationID.x * 16 + 480))));
+}
+
+[numthreads(1, 1, 1)]
+void main(SPIRV_Cross_Input stage_input)
+{
+    gl_GlobalInvocationID = stage_input.gl_GlobalInvocationID;
+    comp_main();
+}

reference/shaders/desktop-only/frag/image-size.frag

@@ -0,0 +1,12 @@
+#version 450
+
+layout(binding = 0, r32f) uniform readonly writeonly image2D uImage1;
+layout(binding = 1, r32f) uniform readonly writeonly image2D uImage2;
+
+layout(location = 0) out vec4 FragColor;
+
+void main()
+{
+    FragColor = vec4(vec2(imageSize(uImage1)), vec2(imageSize(uImage2)));
+}
+

reference/shaders/desktop-only/frag/image-size.no-qualifier-deduction.frag

@@ -0,0 +1,12 @@
+#version 450
+
+layout(binding = 0, r32f) uniform image2D uImage1;
+layout(binding = 1, r32f) uniform image2D uImage2;
+
+layout(location = 0) out vec4 FragColor;
+
+void main()
+{
+    FragColor = vec4(vec2(imageSize(uImage1)), vec2(imageSize(uImage2)));
+}
+

shaders-hlsl/comp/access-chains.force-uav.comp

@@ -0,0 +1,24 @@
+#version 310 es
+layout(local_size_x = 1) in;
+
+// TODO: Read structs, matrices and arrays.
+
+layout(std430, binding = 0) readonly buffer SSBO
+{
+	vec4 a[3][2][4];
+	float b[3][2][4];
+	vec4 unsized[];
+} ro;
+
+layout(std430, binding = 1) writeonly buffer SSBO1
+{
+	vec4 c[3][2][4];
+	float d[3][2][4];
+	vec4 unsized[];
+} wo;
+
+void main()
+{
+	wo.c[2][gl_GlobalInvocationID.x][1] = ro.a[1][gl_GlobalInvocationID.x][2];
+	wo.unsized[gl_GlobalInvocationID.x] = ro.unsized[gl_GlobalInvocationID.x];
+}

shaders/desktop-only/frag/image-size.frag

@@ -0,0 +1,10 @@
+#version 450
+
+layout(location = 0) out vec4 FragColor;
+layout(r32f, set = 0, binding = 0) uniform image2D uImage1;
+layout(r32f, set = 0, binding = 1) uniform image2D uImage2;
+
+void main()
+{
+	FragColor = vec4(imageSize(uImage1), imageSize(uImage2));
+}

shaders/desktop-only/frag/image-size.no-qualifier-deduction.frag

@@ -0,0 +1,10 @@
+#version 450
+
+layout(location = 0) out vec4 FragColor;
+layout(r32f, set = 0, binding = 0) uniform image2D uImage1;
+layout(r32f, set = 0, binding = 1) uniform image2D uImage2;
+
+void main()
+{
+	FragColor = vec4(imageSize(uImage1), imageSize(uImage2));
+}

spirv_cross_c.cpp

@@ -420,6 +420,9 @@ spvc_result spvc_compiler_options_set_uint(spvc_compiler_options options, spvc_c
 	case SPVC_COMPILER_OPTION_EMIT_LINE_DIRECTIVES:
 		options->glsl.emit_line_directives = value != 0;
 		break;
+	case SPVC_COMPILER_OPTION_ENABLE_STORAGE_IMAGE_QUALIFIER_DEDUCTION:
+		options->glsl.enable_storage_image_qualifier_deduction = value != 0;
+		break;
 
 	case SPVC_COMPILER_OPTION_GLSL_SUPPORT_NONZERO_BASE_INSTANCE:
 		options->glsl.vertex.support_nonzero_base_instance = value != 0;
@@ -471,6 +474,10 @@ spvc_result spvc_compiler_options_set_uint(spvc_compiler_options options, spvc_c
 	case SPVC_COMPILER_OPTION_HLSL_SUPPORT_NONZERO_BASE_VERTEX_BASE_INSTANCE:
 		options->hlsl.support_nonzero_base_vertex_base_instance = value != 0;
 		break;
+
+	case SPVC_COMPILER_OPTION_HLSL_FORCE_STORAGE_BUFFER_AS_UAV:
+		options->hlsl.force_storage_buffer_as_uav = value != 0;
+		break;
 #endif
 
 #if SPIRV_CROSS_C_API_MSL

spirv_cross_c.h

@@ -33,7 +33,7 @@ extern "C" {
 /* Bumped if ABI or API breaks backwards compatibility. */
 #define SPVC_C_API_VERSION_MAJOR 0
 /* Bumped if APIs or enumerations are added in a backwards compatible way. */
-#define SPVC_C_API_VERSION_MINOR 25
+#define SPVC_C_API_VERSION_MINOR 27
 /* Bumped if internal implementation details change. */
 #define SPVC_C_API_VERSION_PATCH 0
 
@@ -574,6 +574,10 @@ typedef enum spvc_compiler_option
 	SPVC_COMPILER_OPTION_MSL_FORCE_ACTIVE_ARGUMENT_BUFFER_RESOURCES = 50 | SPVC_COMPILER_OPTION_MSL_BIT,
 	SPVC_COMPILER_OPTION_MSL_FORCE_NATIVE_ARRAYS = 51 | SPVC_COMPILER_OPTION_MSL_BIT,
 
+	SPVC_COMPILER_OPTION_ENABLE_STORAGE_IMAGE_QUALIFIER_DEDUCTION = 52 | SPVC_COMPILER_OPTION_COMMON_BIT,
+
+	SPVC_COMPILER_OPTION_HLSL_FORCE_STORAGE_BUFFER_AS_UAV = 53 | SPVC_COMPILER_OPTION_HLSL_BIT,
+
 	SPVC_COMPILER_OPTION_INT_MAX = 0x7fffffff
 } spvc_compiler_option;
 

spirv_glsl.cpp

@@ -2504,11 +2504,14 @@ void CompilerGLSL::emit_pls()
 
 void CompilerGLSL::fixup_image_load_store_access()
 {
+	if (!options.enable_storage_image_qualifier_deduction)
+		return;
+
 	ir.for_each_typed_id<SPIRVariable>([&](uint32_t var, const SPIRVariable &) {
 		auto &vartype = expression_type(var);
 		if (vartype.basetype == SPIRType::Image)
 		{
-			// Older glslangValidator does not emit required qualifiers here.
+			// Very old glslangValidator and HLSL compilers do not emit required qualifiers here.
 			// Solve this by making the image access as restricted as possible and loosen up if we need to.
 			// If any no-read/no-write flags are actually set, assume that the compiler knows what it's doing.
 

spirv_glsl.hpp

@@ -108,6 +108,13 @@ public:
 		// May not correspond exactly to original source, but should be a good approximation.
 		bool emit_line_directives = false;
 
+		// In cases where readonly/writeonly decoration are not used at all,
+		// we try to deduce which qualifier(s) we should actually used, since actually emitting
+		// read-write decoration is very rare, and older glslang/HLSL compilers tend to just emit readwrite as a matter of fact.
+		// The default (true) is to enable automatic deduction for these cases, but if you trust the decorations set
+		// by the SPIR-V, it's recommended to set this to false.
+		bool enable_storage_image_qualifier_deduction = true;
+
 		enum Precision
 		{
 			DontCare,

spirv_hlsl.cpp

@@ -1857,7 +1857,7 @@ void CompilerHLSL::emit_buffer_block(const SPIRVariable &var)
 	if (is_uav)
 	{
 		Bitset flags = ir.get_buffer_block_flags(var);
-		bool is_readonly = flags.get(DecorationNonWritable);
+		bool is_readonly = flags.get(DecorationNonWritable) && !hlsl_options.force_storage_buffer_as_uav;
 		bool is_coherent = flags.get(DecorationCoherent);
 		bool is_interlocked = interlocked_resources.count(var.self) > 0;
 		const char *type_name = "ByteAddressBuffer ";
@@ -2981,7 +2981,7 @@ string CompilerHLSL::to_resource_binding(const SPIRVariable &var)
 			if (has_decoration(type.self, DecorationBufferBlock))
 			{
 				Bitset flags = ir.get_buffer_block_flags(var);
-				bool is_readonly = flags.get(DecorationNonWritable);
+				bool is_readonly = flags.get(DecorationNonWritable) && !hlsl_options.force_storage_buffer_as_uav;
 				space = is_readonly ? 't' : 'u'; // UAV
 				resource_flags = is_readonly ? HLSL_BINDING_AUTO_SRV_BIT : HLSL_BINDING_AUTO_UAV_BIT;
 			}
@@ -3000,7 +3000,7 @@ string CompilerHLSL::to_resource_binding(const SPIRVariable &var)
 		{
 			// UAV or SRV depending on readonly flag.
 			Bitset flags = ir.get_buffer_block_flags(var);
-			bool is_readonly = flags.get(DecorationNonWritable);
+			bool is_readonly = flags.get(DecorationNonWritable) && !hlsl_options.force_storage_buffer_as_uav;
 			space = is_readonly ? 't' : 'u';
 			resource_flags = is_readonly ? HLSL_BINDING_AUTO_SRV_BIT : HLSL_BINDING_AUTO_UAV_BIT;
 		}

spirv_hlsl.hpp

@@ -109,6 +109,10 @@ public:
 		// Set to false if you know you will never use base instance or base vertex
 		// functionality as it might remove an internal cbuffer.
 		bool support_nonzero_base_vertex_base_instance = false;
+
+		// Forces a storage buffer to always be declared as UAV, even if the readonly decoration is used.
+		// By default, a readonly storage buffer will be declared as ByteAddressBuffer (SRV) instead.
+		bool force_storage_buffer_as_uav = false;
 	};
 
 	explicit CompilerHLSL(std::vector<uint32_t> spirv_)

spirv_parser.cpp

@@ -708,15 +708,6 @@ void Parser::parse(const Instruction &instruction)
 		}
 
 		set<SPIRVariable>(id, type, storage, initializer);
-
-		// hlsl based shaders don't have those decorations. force them and then reset when reading/writing images
-		auto &ttype = get<SPIRType>(type);
-		if (ttype.basetype == SPIRType::BaseType::Image)
-		{
-			ir.set_decoration(id, DecorationNonWritable);
-			ir.set_decoration(id, DecorationNonReadable);
-		}
-
 		break;
 	}
 

test_shaders.py

@@ -358,6 +358,8 @@ def cross_compile_hlsl(shader, spirv, opt, force_no_external_validation, iterati
     hlsl_args = [spirv_cross_path, '--entry', 'main', '--output', hlsl_path, spirv_path, '--hlsl-enable-compat', '--hlsl', '--shader-model', sm, '--iterations', str(iterations)]
     if '.line.' in shader:
         hlsl_args.append('--emit-line-directives')
+    if '.force-uav.' in shader:
+        hlsl_args.append('--hlsl-force-storage-buffer-as-uav')
     subprocess.check_call(hlsl_args)
 
     if not shader_is_invalid_spirv(hlsl_path):
@@ -436,6 +438,8 @@ def cross_compile(shader, vulkan, spirv, invalid_spirv, eliminate, is_legacy, fl
         extra_args += ['--emit-line-directives']
     if '.no-samplerless.' in shader:
         extra_args += ['--vulkan-glsl-disable-ext-samplerless-texture-functions']
+    if '.no-qualifier-deduction.' in shader:
+        extra_args += ['--disable-storage-image-qualifier-deduction']
 
     spirv_cross_path = paths.spirv_cross