Skip to content

Commit

Permalink
GLSL: Fix expression reorder bug with legacy FMA fallback.
Browse files Browse the repository at this point in the history
  • Loading branch information
HansKristian-Work committed Mar 30, 2023
1 parent 8e64f8e commit 50623e1
Show file tree
Hide file tree
Showing 7 changed files with 53 additions and 3 deletions.
4 changes: 3 additions & 1 deletion reference/opt/shaders/legacy/fragment/fma.legacy.frag
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

10 changes: 10 additions & 0 deletions reference/shaders-no-opt/asm/vert/fma-legacy-fallback.asm.vert
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#version 310 es

layout(location = 0) out float a;

void main()
{
a = 5.0;
a = a * 2.0 + 1.0;
}

1 change: 1 addition & 0 deletions reference/shaders/legacy/fragment/fma.legacy.frag
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

24 changes: 24 additions & 0 deletions shaders-no-opt/asm/vert/fma-legacy-fallback.asm.vert
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions shaders/legacy/fragment/fma.legacy.frag
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
15 changes: 13 additions & 2 deletions spirv_glsl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)"))
Expand Down
1 change: 1 addition & 0 deletions spirv_glsl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,7 @@ class CompilerGLSL : public Compiler
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);
Expand Down

0 comments on commit 50623e1

Please sign in to comment.