Merge pull request #2130 from KhronosGroup/fix-2129

GLSL: Fix expression reorder bug with legacy FMA fallback.

reference/opt/shaders/legacy/fragment/fma.legacy.frag

@@ -8,6 +8,8 @@ varying highp vec4 vC;
 
 void main()
 {
-    gl_FragData[0] = vA * vB + vC;
+    highp vec4 _17 = vA * vB + vC;
+    gl_FragData[0] = _17;
+    gl_FragData[0] = _17 * (vB * vC + vA);
 }
 

reference/shaders-no-opt/asm/vert/fma-legacy-fallback.asm.vert

@@ -0,0 +1,10 @@
+#version 310 es
+
+layout(location = 0) out float a;
+
+void main()
+{
+    a = 5.0;
+    a = a * 2.0 + 1.0;
+}
+

reference/shaders/legacy/fragment/fma.legacy.frag

@@ -9,5 +9,6 @@ varying highp vec4 vC;
 void main()
 {
     gl_FragData[0] = vA * vB + vC;
+    gl_FragData[0] = (vA * vB + vC) * (vB * vC + vA);
 }
 

shaders-no-opt/asm/vert/fma-legacy-fallback.asm.vert

@@ -0,0 +1,24 @@
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Vertex %main "main" %a
+               OpSource ESSL 310
+               OpName %main "main"
+               OpName %a "a"
+               OpDecorate %a Location 0
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+%_ptr_Output_float = OpTypePointer Output %float
+          %a = OpVariable %_ptr_Output_float Output
+    %float_5 = OpConstant %float 5
+    %float_2 = OpConstant %float 2
+    %float_1 = OpConstant %float 1
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+               OpStore %a %float_5
+         %10 = OpLoad %float %a
+         %14 = OpExtInst %float %1 Fma %10 %float_2 %float_1
+               OpStore %a %14
+               OpReturn
+               OpFunctionEnd

shaders/legacy/fragment/fma.legacy.frag

@@ -8,4 +8,5 @@ layout(location = 0) out vec4 FragColor;
 void main()
 {
    FragColor = fma(vA, vB, vC);
+   FragColor = fma(vA, vB, vC) * fma(vB, vC, vA);
 }

spirv_glsl.cpp

@@ -4688,7 +4688,7 @@ void CompilerGLSL::strip_enclosed_expression(string &expr)
 	expr.erase(begin(expr));
 }
 
-string CompilerGLSL::enclose_expression(const string &expr)
+bool CompilerGLSL::needs_enclose_expression(const std::string &expr)
 {
 	bool need_parens = false;
 
@@ -4722,10 +4722,15 @@ string CompilerGLSL::enclose_expression(const string &expr)
 		assert(paren_count == 0);
 	}
 
+	return need_parens;
+}
+
+string CompilerGLSL::enclose_expression(const string &expr)
+{
 	// If this expression contains any spaces which are not enclosed by parentheses,
 	// we need to enclose it so we can treat the whole string as an expression.
 	// This happens when two expressions have been part of a binary op earlier.
-	if (need_parens)
+	if (needs_enclose_expression(expr))
 		return join('(', expr, ')');
 	else
 		return expr;
@@ -10897,6 +10902,12 @@ bool CompilerGLSL::optimize_read_modify_write(const SPIRType &type, const string
 
 	char bop = rhs[op];
 	auto expr = rhs.substr(lhs.size() + 3);
+
+	// Avoids false positives where we get a = a * b + c.
+	// Normally, these expressions are always enclosed, but unexpected code paths may end up hitting this.
+	if (needs_enclose_expression(expr))
+		return false;
+
 	// Try to find increments and decrements. Makes it look neater as += 1, -= 1 is fairly rare to see in real code.
 	// Find some common patterns which are equivalent.
 	if ((bop == '+' || bop == '-') && (expr == "1" || expr == "uint(1)" || expr == "1u" || expr == "int(1u)"))

spirv_glsl.hpp

@@ -769,6 +769,7 @@ protected:
 	std::string to_extract_component_expression(uint32_t id, uint32_t index);
 	std::string to_extract_constant_composite_expression(uint32_t result_type, const SPIRConstant &c,
 	                                                     const uint32_t *chain, uint32_t length);
+	static bool needs_enclose_expression(const std::string &expr);
 	std::string enclose_expression(const std::string &expr);
 	std::string dereference_expression(const SPIRType &expression_type, const std::string &expr);
 	std::string address_of_expression(const std::string &expr);