Merge pull request #1676 from KhronosGroup/fix-1671

GLSL: Implement noncoherent framebuffer fetch.

8 files changed, 115 insertions, 8 deletions

diff --git a/main.cpp b/main.cpp
index 953abfad..f3b27431 100644
--- a/main.cpp
+++ b/main.cpp
@@ -662,6 +662,7 @@ struct CLIArguments
 	bool glsl_emit_ubo_as_plain_uniforms = false;
 	bool glsl_force_flattened_io_blocks = false;
 	SmallVector<pair<uint32_t, uint32_t>> glsl_ext_framebuffer_fetch;
+	bool glsl_ext_framebuffer_fetch_noncoherent = false;
 	bool vulkan_glsl_disable_ext_samplerless_texture_functions = false;
 	bool emit_line_directives = false;
 	bool enable_storage_image_qualifier_deduction = true;
@@ -754,6 +755,7 @@ static void print_help_glsl()
 	                "\t[--glsl-emit-ubo-as-plain-uniforms]:\n\t\tInstead of emitting UBOs, emit them as plain uniform structs.\n"
 	                "\t[--glsl-remap-ext-framebuffer-fetch input-attachment color-location]:\n\t\tRemaps an input attachment to use GL_EXT_shader_framebuffer_fetch.\n"
 	                "\t\tgl_LastFragData[location] is read from. The attachment to read from must be declared as an output in the shader.\n"
+	                "\t[--glsl-ext-framebuffer-fetch-noncoherent]:\n\t\tUses noncoherent qualifier for framebuffer fetch.\n"
 	                "\t[--vulkan-glsl-disable-ext-samplerless-texture-functions]:\n\t\tDo not allow use of GL_EXT_samperless_texture_functions, even in Vulkan GLSL.\n"
 	                "\t\tUse of texelFetch and similar might have to create dummy samplers to work around it.\n"
 	                "\t[--combined-samplers-inherit-bindings]:\n\t\tInherit binding information from the textures when building combined image samplers from separate textures and samplers.\n"
@@ -1279,7 +1281,7 @@ static string compile_iteration(const CLIArguments &args, std::vector<uint32_t>
 	compiler->set_common_options(opts);
 
 	for (auto &fetch : args.glsl_ext_framebuffer_fetch)
-		compiler->remap_ext_framebuffer_fetch(fetch.first, fetch.second);
+		compiler->remap_ext_framebuffer_fetch(fetch.first, fetch.second, !args.glsl_ext_framebuffer_fetch_noncoherent);
 
 	// Set HLSL specific options.
 	if (args.hlsl)
@@ -1469,6 +1471,9 @@ static int main_inner(int argc, char *argv[])
 		uint32_t color_attachment = parser.next_uint();
 		args.glsl_ext_framebuffer_fetch.push_back({ input_index, color_attachment });
 	});
+	cbs.add("--glsl-ext-framebuffer-fetch-noncoherent", [&args](CLIParser &) {
+		args.glsl_ext_framebuffer_fetch_noncoherent = 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",
diff --git a/reference/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.framebuffer-fetch-noncoherent.frag b/reference/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.framebuffer-fetch-noncoherent.frag
new file mode 100644
index 00000000..86005498
--- /dev/null
+++ b/reference/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.framebuffer-fetch-noncoherent.frag
@@ -0,0 +1,18 @@
+#version 310 es
+#extension GL_EXT_shader_framebuffer_fetch_non_coherent : require
+precision mediump float;
+precision highp int;
+
+mediump vec4 uSubpass0;
+mediump vec4 uSubpass1;
+
+layout(location = 0, noncoherent) inout vec3 FragColor;
+layout(location = 1, noncoherent) inout vec4 FragColor2;
+
+void main()
+{
+    uSubpass0.xyz = FragColor;
+    uSubpass1 = FragColor2;
+    FragColor = uSubpass0.xyz + uSubpass1.xyz;
+}
+
diff --git a/reference/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.legacy.framebuffer-fetch-noncoherent.frag b/reference/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.legacy.framebuffer-fetch-noncoherent.frag
new file mode 100644
index 00000000..c0a40571
--- /dev/null
+++ b/reference/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.legacy.framebuffer-fetch-noncoherent.frag
@@ -0,0 +1,16 @@
+#version 100
+#extension GL_EXT_shader_framebuffer_fetch_non_coherent : require
+#extension GL_EXT_draw_buffers : require
+precision mediump float;
+precision highp int;
+
+mediump vec4 uSubpass0;
+mediump vec4 uSubpass1;
+
+void main()
+{
+    uSubpass0 = gl_LastFragData[0];
+    uSubpass1 = gl_LastFragData[1];
+    gl_FragData[0] = uSubpass0.xyz + uSubpass1.xyz;
+}
+
diff --git a/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.framebuffer-fetch-noncoherent.frag b/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.framebuffer-fetch-noncoherent.frag
new file mode 100644
index 00000000..621457a1
--- /dev/null
+++ b/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.framebuffer-fetch-noncoherent.frag
@@ -0,0 +1,12 @@
+#version 310 es
+precision mediump float;
+
+layout(input_attachment_index = 0, set = 0, binding = 0) uniform mediump subpassInput uSubpass0;
+layout(input_attachment_index = 1, set = 0, binding = 1) uniform mediump subpassInput uSubpass1;
+layout(location = 0) out vec3 FragColor;
+layout(location = 1) out vec4 FragColor2;
+
+void main()
+{
+    FragColor.rgb = subpassLoad(uSubpass0).rgb + subpassLoad(uSubpass1).rgb;
+}
diff --git a/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.legacy.framebuffer-fetch-noncoherent.frag b/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.legacy.framebuffer-fetch-noncoherent.frag
new file mode 100644
index 00000000..621457a1
--- /dev/null
+++ b/shaders-no-opt/frag/subpass-input.framebuffer-fetch.nocompat.legacy.framebuffer-fetch-noncoherent.frag
@@ -0,0 +1,12 @@
+#version 310 es
+precision mediump float;
+
+layout(input_attachment_index = 0, set = 0, binding = 0) uniform mediump subpassInput uSubpass0;
+layout(input_attachment_index = 1, set = 0, binding = 1) uniform mediump subpassInput uSubpass1;
+layout(location = 0) out vec3 FragColor;
+layout(location = 1) out vec4 FragColor2;
+
+void main()
+{
+    FragColor.rgb = subpassLoad(uSubpass0).rgb + subpassLoad(uSubpass1).rgb;
+}
diff --git a/spirv_glsl.cpp b/spirv_glsl.cpp
index a8d0925f..6feac01a 100644
--- a/spirv_glsl.cpp
+++ b/spirv_glsl.cpp
@@ -359,10 +359,26 @@ void CompilerGLSL::remap_pls_variables()
 	}
 }
 
-void CompilerGLSL::remap_ext_framebuffer_fetch(uint32_t input_attachment_index, uint32_t color_location)
+void CompilerGLSL::remap_ext_framebuffer_fetch(uint32_t input_attachment_index, uint32_t color_location, bool coherent)
 {
 	subpass_to_framebuffer_fetch_attachment.push_back({ input_attachment_index, color_location });
-	inout_color_attachments.insert(color_location);
+	inout_color_attachments.push_back({ color_location, coherent });
+}
+
+bool CompilerGLSL::location_is_framebuffer_fetch(uint32_t location) const
+{
+	return std::find_if(begin(inout_color_attachments), end(inout_color_attachments),
+	                    [&](const std::pair<uint32_t, bool> &elem) {
+		                    return elem.first == location;
+	                    }) != end(inout_color_attachments);
+}
+
+bool CompilerGLSL::location_is_non_coherent_framebuffer_fetch(uint32_t location) const
+{
+	return std::find_if(begin(inout_color_attachments), end(inout_color_attachments),
+	                    [&](const std::pair<uint32_t, bool> &elem) {
+		                    return elem.first == location && !elem.second;
+	                    }) != end(inout_color_attachments);
 }
 
 void CompilerGLSL::find_static_extensions()
@@ -484,7 +500,22 @@ void CompilerGLSL::find_static_extensions()
 			SPIRV_CROSS_THROW("Can only use GL_EXT_shader_framebuffer_fetch in fragment shaders.");
 		if (options.vulkan_semantics)
 			SPIRV_CROSS_THROW("Cannot use EXT_shader_framebuffer_fetch in Vulkan GLSL.");
-		require_extension_internal("GL_EXT_shader_framebuffer_fetch");
+
+		bool has_coherent = false;
+		bool has_incoherent = false;
+
+		for (auto &att : inout_color_attachments)
+		{
+			if (att.second)
+				has_coherent = true;
+			else
+				has_incoherent = true;
+		}
+
+		if (has_coherent)
+			require_extension_internal("GL_EXT_shader_framebuffer_fetch");
+		if (has_incoherent)
+			require_extension_internal("GL_EXT_shader_framebuffer_fetch_non_coherent");
 	}
 
 	if (options.separate_shader_objects && !options.es && options.version < 410)
@@ -1693,6 +1724,12 @@ string CompilerGLSL::layout_for_variable(const SPIRVariable &var)
 			attr.push_back(join("location = ", get_decoration(var.self, DecorationLocation)));
 	}
 
+	if (get_execution_model() == ExecutionModelFragment && var.storage == StorageClassOutput &&
+	    location_is_non_coherent_framebuffer_fetch(get_decoration(var.self, DecorationLocation)))
+	{
+		attr.push_back("noncoherent");
+	}
+
 	// Transform feedback
 	bool uses_enhanced_layouts = false;
 	if (is_block && var.storage == StorageClassOutput)
@@ -2234,7 +2271,9 @@ const char *CompilerGLSL::to_storage_qualifiers_glsl(const SPIRVariable &var)
 			return "varying "; // Fragment outputs are renamed so they never hit this case.
 		else if (execution.model == ExecutionModelFragment && var.storage == StorageClassOutput)
 		{
-			if (inout_color_attachments.count(get_decoration(var.self, DecorationLocation)) != 0)
+			uint32_t loc = get_decoration(var.self, DecorationLocation);
+			bool is_inout = location_is_framebuffer_fetch(loc);
+			if (is_inout)
 				return "inout ";
 			else
 				return "out ";
@@ -3405,7 +3444,7 @@ void CompilerGLSL::emit_resources()
 
 		// Unused output I/O variables might still be required to implement framebuffer fetch.
 		if (var.storage == StorageClassOutput && !is_legacy() &&
-		    inout_color_attachments.count(get_decoration(var.self, DecorationLocation)) != 0)
+		    location_is_framebuffer_fetch(get_decoration(var.self, DecorationLocation)) != 0)
 		{
 			is_hidden = false;
 		}
diff --git a/spirv_glsl.hpp b/spirv_glsl.hpp
index a3501ca9..20b94935 100644
--- a/spirv_glsl.hpp
+++ b/spirv_glsl.hpp
@@ -178,7 +178,8 @@ public:
 
 	// Redirect a subpassInput reading from input_attachment_index to instead load its value from
 	// the color attachment at location = color_location. Requires ESSL.
-	void remap_ext_framebuffer_fetch(uint32_t input_attachment_index, uint32_t color_location);
+	// If coherent, uses GL_EXT_shader_framebuffer_fetch, if not, uses noncoherent variant.
+	void remap_ext_framebuffer_fetch(uint32_t input_attachment_index, uint32_t color_location, bool coherent);
 
 	explicit CompilerGLSL(std::vector<uint32_t> spirv_)
 	    : Compiler(std::move(spirv_))
@@ -858,7 +859,9 @@ protected:
 
 	// GL_EXT_shader_framebuffer_fetch support.
 	std::vector<std::pair<uint32_t, uint32_t>> subpass_to_framebuffer_fetch_attachment;
-	std::unordered_set<uint32_t> inout_color_attachments;
+	std::vector<std::pair<uint32_t, bool>> inout_color_attachments;
+	bool location_is_framebuffer_fetch(uint32_t location) const;
+	bool location_is_non_coherent_framebuffer_fetch(uint32_t location) const;
 	bool subpass_input_is_framebuffer_fetch(uint32_t id) const;
 	void emit_inout_fragment_outputs_copy_to_subpass_inputs();
 	const SPIRVariable *find_subpass_input_by_attachment_index(uint32_t index) const;
diff --git a/test_shaders.py b/test_shaders.py
index e5906933..9e3b8e44 100755
--- a/test_shaders.py
+++ b/test_shaders.py
@@ -551,6 +551,8 @@ def cross_compile(shader, vulkan, spirv, invalid_spirv, eliminate, is_legacy, fl
         extra_args += ['--glsl-remap-ext-framebuffer-fetch', '1', '1']
         extra_args += ['--glsl-remap-ext-framebuffer-fetch', '2', '2']
         extra_args += ['--glsl-remap-ext-framebuffer-fetch', '3', '3']
+    if '.framebuffer-fetch-noncoherent.' in shader:
+        extra_args += ['--glsl-ext-framebuffer-fetch-noncoherent']
     if '.zero-initialize.' in shader:
         extra_args += ['--force-zero-initialized-variables']
     if '.force-flattened-io.' in shader: