From d4cfe5781284551d7c5ccf2cf1d28a86211bb1df Mon Sep 17 00:00:00 2001 From: Tim Foley Date: Sun, 23 Jul 2017 18:22:30 -0700 Subject: Work around glslang issue 988 The basic bug there is that if you have a member of `struct` type in a `uniform` block and then pass a reference to that member directly to a call: ``` struct Foo { vec4 bar; }; uniform U { Foo foo; }; void main() { doSomething(foo); } ``` then glslang generates invalid SPIR-V which seems to cause an issue for some drivers. This change works around the problem by detecting cases where an argument to a function call is a reference to `uniform` block member (of `struct` type) and then rewrites the code to move that value to a temporary before the call. --- source/slang/lower.cpp | 138 +++++++++++++++++++++--- tests/rewriter/glslang-bug-988-workaround.frag | 58 ++++++++++ tests/rewriter/glslang-bug-988-workaround.slang | 6 ++ tests/rewriter/resources-in-structs.glsl | 3 +- 4 files changed, 192 insertions(+), 13 deletions(-) create mode 100644 tests/rewriter/glslang-bug-988-workaround.frag create mode 100644 tests/rewriter/glslang-bug-988-workaround.slang diff --git a/source/slang/lower.cpp b/source/slang/lower.cpp index 9c2c38b60..d634b114b 100644 --- a/source/slang/lower.cpp +++ b/source/slang/lower.cpp @@ -733,6 +733,18 @@ struct LoweringVisitor return result; } + RefPtr moveTemp(RefPtr expr) + { + RefPtr varDecl = new Variable(); + varDecl->Name.Content = generateName(); + varDecl->Type.type = expr->Type.type; + varDecl->Expr = expr; + + addDecl(varDecl); + + return createVarRef(expr->Position, varDecl); + } + // The idea of this function is to take an expression that we plan to // use/evaluate more than once, and if needed replace it with a // reference to a temporary (initialized with the expr) so that it @@ -769,14 +781,7 @@ struct LoweringVisitor // TODO: handle the tuple cases here... // In the general case, though, we need to introduce a temporary - RefPtr varDecl = new Variable(); - varDecl->Name.Content = generateName(); - varDecl->Type.type = expr->Type.type; - varDecl->Expr = expr; - - addDecl(varDecl); - - return createVarRef(expr->Position, varDecl); + return moveTemp(expr); } // Similar to the above, this ensures that an l-value expression @@ -1289,11 +1294,11 @@ struct LoweringVisitor // Default case: just reconstrut a subscript expr auto loweredExpr = new IndexExpressionSyntaxNode(); - loweredExpr->Type.type = getSubscripResultType(baseExpr->Type.type); +loweredExpr->Type.type = getSubscripResultType(baseExpr->Type.type); - loweredExpr->BaseExpression = baseExpr; - loweredExpr->IndexExpression = indexExpr; - return loweredExpr; +loweredExpr->BaseExpression = baseExpr; +loweredExpr->IndexExpression = indexExpr; +return loweredExpr; } } @@ -1358,6 +1363,84 @@ struct LoweringVisitor return expr; } + bool needGlslangBug988Workaround( + RefPtr inExpr) + { + switch (getTarget()) + { + default: + return false; + + case CodeGenTarget::GLSL: + break; + } + + // There are two conditions we care about here: + // + // (1) is the *type* of the expression something that needs the WAR + // (2) does the expression reference a constant-buffer member? + // + + // Issue (1): is the type of the expression something that needs the WAR? + + auto exprType = inExpr->Type.type; + exprType = unwrapArray(exprType); + + if (!isStructType(exprType)) + return false; + + + // Issue (2): does the expression reference a constant-buffer member? + + auto expr = inExpr; + for (;;) + { + if (auto memberRefExpr = expr.As()) + { + expr = memberRefExpr->BaseExpression; + continue; + } + + if (auto derefExpr = expr.As()) + { + expr = derefExpr->base; + continue; + } + + if (auto subscriptExpr = expr.As()) + { + expr = subscriptExpr->BaseExpression; + continue; + } + + break; + } + + if (auto varExpr = expr.As()) + { + auto declRef = varExpr->declRef; + if (!declRef) + return false; + + if (auto varDeclRef = declRef.As()) + { + auto varType = GetType(varDeclRef); + + while (auto arrayType = varType->As()) + { + varType = arrayType->BaseType; + } + + if (auto constantBufferType = varType->As()) + { + return true; + } + } + } + + return false; + } + void addArgs( ExprWithArgsBase* callExpr, RefPtr argExpr) @@ -1382,6 +1465,17 @@ struct LoweringVisitor } else { + // This should be the default case where we have a perfectly + // ordinary expression, but we need to work around a glslang + // but here, where passing a member of a `uniform` block + // that has `struct` type directly to a function call causes + // invalid SPIR-V to be generated. + if (needGlslangBug988Workaround(argExpr)) + { + argExpr = moveTemp(argExpr); + } + + // Here's the actual default case where we just add an argment callExpr->Arguments.Add(argExpr); } } @@ -2981,8 +3075,28 @@ struct LoweringVisitor } } + AggTypeDecl* isStructType(RefPtr type) + { + if (type->As()) return nullptr; + else if (type->As()) return nullptr; + else if (type->As()) return nullptr; + else if (type->As()) return nullptr; + else if (type->As()) return nullptr; + else if (auto declRefType = type->As()) + { + if (auto aggTypeDeclRef = declRefType->declRef.As()) + { + return aggTypeDeclRef.getDecl(); + } + } + + return nullptr; + } + bool isImportedStructType(RefPtr type) { + // TODO: make this use `isStructType` above + if (type->As()) return false; else if (type->As()) return false; else if (type->As()) return false; diff --git a/tests/rewriter/glslang-bug-988-workaround.frag b/tests/rewriter/glslang-bug-988-workaround.frag new file mode 100644 index 000000000..8c9692aed --- /dev/null +++ b/tests/rewriter/glslang-bug-988-workaround.frag @@ -0,0 +1,58 @@ +#version 450 +//TEST:COMPARE_GLSL: + +// Test workaround for glslang issue #988 +// (https://github.com/KhronosGroup/glslang/issues/988) + + +#if defined(__SLANG__) + + +__import glslang_bug_988_workaround; + +uniform U +{ + Foo foo; +}; + +vec4 doIt(Foo foo) +{ + return foo.bar; +} + +layout(location = 0) +out vec4 result; + +void main() +{ + result = doIt(foo); +} + +#else + +struct Foo +{ + vec4 bar; +}; + +layout(binding = 0) +uniform U +{ + Foo foo; +}; + +vec4 doIt(Foo foo) +{ + return foo.bar; +} + +layout(location = 0) +out vec4 result; + +void main() +{ + Foo SLANG_tmp_0 = foo; + result = doIt(SLANG_tmp_0); +} + +#endif diff --git a/tests/rewriter/glslang-bug-988-workaround.slang b/tests/rewriter/glslang-bug-988-workaround.slang new file mode 100644 index 000000000..841375923 --- /dev/null +++ b/tests/rewriter/glslang-bug-988-workaround.slang @@ -0,0 +1,6 @@ +//TEST_IGNORE_FILE: + +struct Foo +{ + float4 bar; +}; diff --git a/tests/rewriter/resources-in-structs.glsl b/tests/rewriter/resources-in-structs.glsl index 6273e8720..206e4a8d8 100644 --- a/tests/rewriter/resources-in-structs.glsl +++ b/tests/rewriter/resources-in-structs.glsl @@ -55,8 +55,9 @@ out vec4 color; void main() { + Material SLANG_tmp_0 = m; color = evaluateMaterial( - m, + SLANG_tmp_0, SLANG_parameterBlock_U_m_t, SLANG_parameterBlock_U_m_s, uv); } -- cgit v1.2.3