Merge pull request #142 from tfoleyNV/glslang-bug-workaround

Work around glslang issue 988

source/slang/lower.cpp

@@ -733,6 +733,18 @@ struct LoweringVisitor
         return result;
     }
 
+    RefPtr<ExpressionSyntaxNode> moveTemp(RefPtr<ExpressionSyntaxNode> expr)
+    {
+        RefPtr<Variable> 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<Variable> 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<ExpressionSyntaxNode> 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<MemberExpressionSyntaxNode>())
+            {
+                expr = memberRefExpr->BaseExpression;
+                continue;
+            }
+
+            if (auto derefExpr = expr.As<DerefExpr>())
+            {
+                expr = derefExpr->base;
+                continue;
+            }
+
+            if (auto subscriptExpr = expr.As<IndexExpressionSyntaxNode>())
+            {
+                expr = subscriptExpr->BaseExpression;
+                continue;
+            }
+
+            break;
+        }
+
+        if (auto varExpr = expr.As<VarExpressionSyntaxNode>())
+        {
+            auto declRef = varExpr->declRef;
+            if (!declRef)
+                return false;
+
+            if (auto varDeclRef = declRef.As<Variable>())
+            {
+                auto varType = GetType(varDeclRef);
+
+                while (auto arrayType = varType->As<ArrayExpressionType>())
+                {
+                    varType = arrayType->BaseType;
+                }
+
+                if (auto constantBufferType = varType->As<ConstantBufferType>())
+                {
+                    return true;
+                }
+            }
+        }
+
+        return false;
+    }
+
     void addArgs(
         ExprWithArgsBase*               callExpr,
         RefPtr<ExpressionSyntaxNode>    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<ExpressionType> type)
+    {
+        if (type->As<BasicExpressionType>()) return nullptr;
+        else if (type->As<VectorExpressionType>()) return nullptr;
+        else if (type->As<MatrixExpressionType>()) return nullptr;
+        else if (type->As<ResourceType>()) return nullptr;
+        else if (type->As<BuiltinGenericType>()) return nullptr;
+        else if (auto declRefType = type->As<DeclRefType>())
+        {
+            if (auto aggTypeDeclRef = declRefType->declRef.As<AggTypeDecl>())
+            {
+                return aggTypeDeclRef.getDecl();
+            }
+        }
+
+        return nullptr;
+    }
+
     bool isImportedStructType(RefPtr<ExpressionType> type)
     {
+        // TODO: make this use `isStructType` above
+
         if (type->As<BasicExpressionType>()) return false;
         else if (type->As<VectorExpressionType>()) return false;
         else if (type->As<MatrixExpressionType>()) return false;

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

tests/rewriter/glslang-bug-988-workaround.slang

@@ -0,0 +1,6 @@
+//TEST_IGNORE_FILE:
+
+struct Foo
+{
+	float4 bar;
+};

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);
 }