From d4cfe5781284551d7c5ccf2cf1d28a86211bb1df Mon Sep 17 00:00:00 2001
From: Tim Foley <tfoley@nvidia.com>
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 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 126 insertions(+), 12 deletions(-)

(limited to 'source/slang/lower.cpp')

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<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;
-- 
cgit v1.2.3