IR/Vulkan fixes (#412)

* Fix bugs around IR legalization of GLSL input/output

- Add case to handle assignment of one `ScalarizedVal::Flavor::address` to another (still need to make sure we are handling all the possible cases there)

- Revamp logic for creating global variable declarations for varying inputs/outputs.
  - Actually handle creating array declarations (not sure if binding locations will be correct)
  - Properly deal with offsetting of locations for nested fields
  - Only create varying input/output layout information as needed for the separate `in` and `out` variables we create to represent a single HLSL `inout` varying

* During SSA generation, recursively remove trivial phis

This is actually written up in the original paper I used as a reference, but I hadn't implemented the case yet.
When you eliminate one phi as trivial (because its only operands were itself and at most one other value), you might find that another phi becomes trivial (because it had this phi as an operand, but now it will have the other value...).

The one thing that made any of this tricky is that our "phi" nodes are really block parameters, and thus they don't technically have operands (`IRUse`s). The `IRUse`s for each phi were being tracked in a separate array, and had their `user` field set to null.

With this change, I set their `user` to be the corresponding `IRParam` for the phi (and that means I changed `IRParam` to inherit from `IRUser` even though it shouldn't really be required).

* Re-build SSA form after specialization/legalization

The main reason to do this is that legalization might scalarize types, and thus might allow us to clean up resource-type local variables that we were not able to clean up when they were part of an aggregate.

Note: we shouldn't really need to do this, because the front-end should actually be guaranteeing that types that include resources are used in "safe" ways, but we currently don't have the analyses required to support that.

* Give an error message if we get GLSL input

The API and command-line interface still recognize and nominally support GLSL input files, because they need to be supported in the "pass-through" mode.
This change just adds an error message if we encounter a GLSL input file in anything other than "pass-through" mode.

10 files changed, 182 insertions, 89 deletions

diff --git a/source/slang/diagnostic-defs.h b/source/slang/diagnostic-defs.h
index 698591d35..82aed2d69 100644
--- a/source/slang/diagnostic-defs.h
+++ b/source/slang/diagnostic-defs.h
@@ -68,6 +68,9 @@ DIAGNOSTIC(    6, Error, cannotDeduceOutputFormatFromPath,
 DIAGNOSTIC(    6, Error, explicitOutputPathsAndMultipleTargets,
     "canot use both explicit output paths ('-o') and multiple targets ('-target')")
 
+DIAGNOSTIC(    7, Error, glslIsNotSupported,
+    "the Slang compiler does not support GLSL as a source language");
+
 //
 // 1xxxx - Lexical anaylsis
 //
diff --git a/source/slang/emit.cpp b/source/slang/emit.cpp
index a27492c0b..0cd896ced 100644
--- a/source/slang/emit.cpp
+++ b/source/slang/emit.cpp
@@ -2,6 +2,7 @@
 #include "emit.h"
 
 #include "ir-insts.h"
+#include "ir-ssa.h"
 #include "legalize-types.h"
 #include "lower-to-ir.h"
 #include "mangle.h"
@@ -7212,15 +7213,10 @@ emitDeclImpl(decl, nullptr);
     }
 #endif
 
-    void emitIRVarModifiers(
+    void emitIRMatrixLayoutModifiers(
         EmitContext*    ctx,
         VarLayout*      layout)
     {
-        if (!layout)
-            return;
-
-        auto target = ctx->shared->target;
-
         // We need to handle the case where the variable has
         // a matrix type, and has been given a non-standard
         // layout attribute (for HLSL, `row_major` is the
@@ -7228,6 +7224,8 @@ emitDeclImpl(decl, nullptr);
         //
         if (auto matrixTypeLayout = layout->typeLayout.As<MatrixTypeLayout>())
         {
+            auto target = ctx->shared->target;
+
             switch (target)
             {
             case CodeGenTarget::HLSL:
@@ -7269,6 +7267,17 @@ emitDeclImpl(decl, nullptr);
             
         }
 
+    }
+
+    void emitIRVarModifiers(
+        EmitContext*    ctx,
+        VarLayout*      layout)
+    {
+        if (!layout)
+            return;
+
+        emitIRMatrixLayoutModifiers(ctx, layout);
+
         if (ctx->shared->target == CodeGenTarget::GLSL)
         {
             // Layout-related modifiers need to come before the declaration,
@@ -7539,7 +7548,19 @@ emitDeclImpl(decl, nullptr);
                     if(fieldType->Equals(getSession()->getVoidType()))
                         continue;
 
-                    emitIRVarModifiers(ctx, fieldLayout);
+                    // Note: we will emit matrix-layout modifiers here, but
+                    // we will refrain from emitting other modifiers that
+                    // might not be appropriate to the context (e.g., we
+                    // shouldn't go emitting `uniform` just because these
+                    // things are uniform...).
+                    //
+                    // TODO: we need a more refined set of modifiers that
+                    // we should allow on fields, because we might end
+                    // up supporting layout that isn't the default for
+                    // the given block type (e.g., something other than
+                    // `std140` for a uniform block).
+                    //
+                    emitIRMatrixLayoutModifiers(ctx, fieldLayout);
 
                     emitIRType(ctx, fieldType, getIRName(ff));
 
@@ -8240,6 +8261,14 @@ String emitEntryPoint(
         fprintf(stderr, "###\n");
 #endif
 
+        // Once specialization and type legalization have been performed,
+        // we should perform some of our basic optimization steps again,
+        // to see if we can clean up any temporaries created by legalization.
+        // (e.g., things that used to be aggregated might now be split up,
+        // so that we can work with the individual fields).
+        constructSSA(irModule);
+
+
         // After all of the required optimization and legalization
         // passes have been performed, we can emit target code from
         // the IR module.
diff --git a/source/slang/ir-ssa.cpp b/source/slang/ir-ssa.cpp
index 432dfc1b1..53eefd81f 100644
--- a/source/slang/ir-ssa.cpp
+++ b/source/slang/ir-ssa.cpp
@@ -89,7 +89,9 @@ struct ConstructSSAContext
 
     PhiInfo* getPhiInfo(IRParam* phi)
     {
-        return *phiInfos.TryGetValue(phi);
+        if(auto found = phiInfos.TryGetValue(phi))
+            return *found;
+        return nullptr;
     }
 };
 
@@ -219,7 +221,7 @@ PhiInfo* addPhi(
 }
 
 IRValue* tryRemoveTrivialPhi(
-    ConstructSSAContext*    /*context*/,
+    ConstructSSAContext*    context,
     PhiInfo*                phiInfo)
 {
     auto phi = phiInfo->phi;
@@ -266,6 +268,26 @@ IRValue* tryRemoveTrivialPhi(
         assert(!"unimplemented");
     }
 
+    // Removing this phi as trivial may make other phi nodes
+    // become trivial. We will recognize such candidates
+    // by looking for phi nodes that use this node.
+    List<PhiInfo*> otherPhis;
+    for( auto u = phi->firstUse; u; u = u->nextUse )
+    {
+        auto user = u->user;
+        if(!user) continue;
+        if(user == phi) continue;
+
+        if( user->op == kIROp_Param )
+        {
+            auto maybeOtherPhi = (IRParam*) user;
+            if( auto otherPhiInfo = context->getPhiInfo(maybeOtherPhi) )
+            {
+                otherPhis.Add(otherPhiInfo);
+            }
+        }
+    }
+
     // replace uses of the phi (including its possible uses
     // of itself) with the unique non-phi value.
     phi->replaceUsesWith(same);
@@ -281,8 +303,12 @@ IRValue* tryRemoveTrivialPhi(
     // phi, so that we can easily look it up later.
     phiInfo->replacement = same;
 
-    // TODO: need to recursively consider chances to simplify
-    // other phi nodes now.
+    // Now that we've cleaned up this phi, we need to consider
+    // other phis that might have become  trivial.
+    for( auto otherPhi : otherPhis )
+    {
+        tryRemoveTrivialPhi(context, otherPhi);
+    }
 
     return same;
 }
@@ -320,7 +346,7 @@ IRValue* addPhiOperands(
     phiInfo->operands.SetSize(operandCount);
     for(UInt ii = 0; ii < operandCount; ++ii)
     {
-        phiInfo->operands[ii].init(nullptr, operandValues[ii]);
+        phiInfo->operands[ii].init(phiInfo->phi, operandValues[ii]);
     }
 
     return tryRemoveTrivialPhi(context, phiInfo);
diff --git a/source/slang/ir.cpp b/source/slang/ir.cpp
index 50b0b83e9..3d04e9807 100644
--- a/source/slang/ir.cpp
+++ b/source/slang/ir.cpp
@@ -2867,47 +2867,82 @@ namespace Slang
         };
 
         Flavor                      flavor;
-        Val*                        elementCount;
+        IntVal*                     elementCount;
         GlobalVaryingDeclarator*    next;
     };
 
     ScalarizedVal createSimpleGLSLGlobalVarying(
         IRBuilder*                  builder,
-        Type*                       type,
-        VarLayout*                  varLayout,
-        TypeLayout*                 /*typeLayout*/,
-        LayoutResourceKind          /*kind*/,
-        GlobalVaryingDeclarator*    /*declarator*/)
-    {
-        // TODO: We might be creating an `in` or `out` variable based on
-        // an `in out` function parameter. In this case we should
-        // rewrite the `typeLayout` to only include the information
-        // for the appropriate `kind`.
-        //
-        // TODO: actually, we should *always* be re-creating the layout,
-        // because we need to apply any offsets from the parent...
-
-        // TODO: If there are any `declarator`s, we need to unwrap
-        // them here, and allow them to modify the type of the
-        // variable that we declare.
-        //
-        // They should probably also affect how we return the
-        // `ScalarizedVal`, since we need to reflect the AOS->SOA conversion.
-
+        Type*                       inType,
+        VarLayout*                  inVarLayout,
+        TypeLayout*                 inTypeLayout,
+        LayoutResourceKind          kind,
+        UInt                        bindingIndex,
+        GlobalVaryingDeclarator*    declarator)
+    {
         // TODO: detect when the layout represents a system input/output
-        if( varLayout->systemValueSemantic.Length() != 0 )
+        if( inVarLayout->systemValueSemantic.Length() != 0 )
         {
             // This variable represents a system input/output,
             // and we should probably handle that differently, right?
         }
 
+        RefPtr<Type> type = inType;
+        RefPtr<TypeLayout> typeLayout = inTypeLayout;
+        for( auto dd = declarator; dd; dd = dd->next )
+        {
+            assert(dd->flavor == GlobalVaryingDeclarator::Flavor::array);
+
+            RefPtr<ArrayExpressionType> arrayType = builder->getSession()->getArrayType(
+                type,
+                dd->elementCount);
+
+            RefPtr<ArrayTypeLayout> arrayTypeLayout = new ArrayTypeLayout();
+            arrayTypeLayout->type = arrayType;
+            arrayTypeLayout->rules = typeLayout->rules;
+            arrayTypeLayout->originalElementTypeLayout =  typeLayout;
+            arrayTypeLayout->elementTypeLayout = typeLayout;
+            arrayTypeLayout->uniformStride = 0;
+
+            if( auto resInfo = inTypeLayout->FindResourceInfo(kind) )
+            {
+                // TODO: it is kind of gross to be re-running some
+                // of the type layout logic here.
+
+                UInt elementCount = (UInt) GetIntVal(dd->elementCount);
+                arrayTypeLayout->addResourceUsage(
+                    kind,
+                    resInfo->count * elementCount);
+            }
+
+            type = arrayType;
+            typeLayout = arrayTypeLayout;
+        }
+
         // Simple case: just create a global variable of the matching type,
         // and then use the value of the global as a replacement for the
         // value of the original parameter.
         //
         auto globalVariable = addGlobalVariable(builder->getModule(), type);
         moveValueBefore(globalVariable, builder->getFunc());
+
+        // We need to construct a fresh layout for the variable, even
+        // if the original had its own layout, because it might be
+        // an `inout` parameter, and we only want to deal with the case
+        // described by our `kind` parameter.
+        RefPtr<VarLayout> varLayout = new VarLayout();
+        varLayout->varDecl = inVarLayout->varDecl;
+        varLayout->typeLayout = typeLayout;
+        varLayout->flags = inVarLayout->flags;
+        varLayout->systemValueSemantic = inVarLayout->systemValueSemantic;
+        varLayout->systemValueSemanticIndex = inVarLayout->systemValueSemanticIndex;
+        varLayout->semanticName = inVarLayout->semanticName;
+        varLayout->semanticIndex = inVarLayout->semanticIndex;
+        varLayout->stage = inVarLayout->stage;
+        varLayout->AddResourceInfo(kind)->index = bindingIndex;
+
         builder->addLayoutDecoration(globalVariable, varLayout);
+
         return ScalarizedVal::address(globalVariable);
     }
 
@@ -2917,20 +2952,21 @@ namespace Slang
         VarLayout*                  varLayout,
         TypeLayout*                 typeLayout,
         LayoutResourceKind          kind,
+        UInt                        bindingIndex,
         GlobalVaryingDeclarator*    declarator)
     {
         if( type->As<BasicExpressionType>() )
         {
-            return createSimpleGLSLGlobalVarying(builder, type, varLayout, typeLayout, kind, declarator);
+            return createSimpleGLSLGlobalVarying(builder, type, varLayout, typeLayout, kind, bindingIndex, declarator);
         }
         else if( type->As<VectorExpressionType>() )
         {
-            return createSimpleGLSLGlobalVarying(builder, type, varLayout, typeLayout, kind, declarator);
+            return createSimpleGLSLGlobalVarying(builder, type, varLayout, typeLayout, kind, bindingIndex, declarator);
         }
         else if( type->As<MatrixExpressionType>() )
         {
             // TODO: a matrix-type varying should probably be handled like an array of rows
-            return createSimpleGLSLGlobalVarying(builder, type, varLayout, typeLayout, kind, declarator);
+            return createSimpleGLSLGlobalVarying(builder, type, varLayout, typeLayout, kind, bindingIndex, declarator);
         }
         else if( auto arrayType = type->As<ArrayExpressionType>() )
         {
@@ -2953,6 +2989,7 @@ namespace Slang
                 varLayout,
                 elementTypeLayout,
                 kind,
+                bindingIndex,
                 &arrayDeclarator);
         }
         else if( auto declRefType = type->As<DeclRefType>() )
@@ -2978,12 +3015,17 @@ namespace Slang
                     // generate one variable for each.
                     for( auto ff : structTypeLayout->fields )
                     {
+                        UInt fieldBindingIndex = bindingIndex;
+                        if(auto fieldResInfo = ff->FindResourceInfo(kind))
+                            fieldBindingIndex += fieldResInfo->index;
+
                         auto fieldVal = createGLSLGlobalVaryingsImpl(
                             builder,
                             ff->typeLayout->type,
                             ff,
                             ff->typeLayout,
                             kind,
+                            fieldBindingIndex,
                             declarator);
 
                         ScalarizedTupleValImpl::Element element;
@@ -2999,7 +3041,7 @@ namespace Slang
         }
 
         // Default case is to fall back on the simple behavior
-        return createSimpleGLSLGlobalVarying(builder, type, varLayout, typeLayout, kind, declarator);
+        return createSimpleGLSLGlobalVarying(builder, type, varLayout, typeLayout, kind, bindingIndex, declarator);
     }
 
     ScalarizedVal createGLSLGlobalVaryings(
@@ -3008,7 +3050,10 @@ namespace Slang
         VarLayout*          layout,
         LayoutResourceKind  kind)
     {
-        return createGLSLGlobalVaryingsImpl(builder, type, layout, layout->typeLayout, kind, nullptr);
+        UInt bindingIndex = 0;
+        if(auto rr = layout->FindResourceInfo(kind))
+            bindingIndex = rr->index;
+        return createGLSLGlobalVaryingsImpl(builder, type, layout, layout->typeLayout, kind, bindingIndex, nullptr);
     }
 
     ScalarizedVal extractField(
@@ -3062,6 +3107,13 @@ namespace Slang
                 }
                 break;
 
+            case ScalarizedVal::Flavor::address:
+                {
+                    auto val = builder->emitLoad(right.irValue);
+                    builder->emitStore(left.irValue, val);
+                }
+                break;
+
             default:
                 SLANG_UNEXPECTED("unimplemented");
                 break;
diff --git a/source/slang/ir.h b/source/slang/ir.h
index 407506116..0dc9daa9a 100644
--- a/source/slang/ir.h
+++ b/source/slang/ir.h
@@ -256,7 +256,7 @@ struct IRUser : IRChildValue
     // instructions that need "vararg" support to
     // allocate this field ahead of the `this`
     // pointer.
-    uint32_t argCount;
+    uint32_t argCount = 0;
 
     UInt getArgCount()
     {
@@ -333,7 +333,10 @@ bool isTerminatorInst(IRInst* inst);
 //
 // In each case, the basic idea is that a block is a "label with
 // arguments."
-struct IRParam : IRValue
+//
+// Note: an `IRParam` is an `IRUser` *just* so that we can use
+// it as the user of other values during SSA generation.
+struct IRParam : IRUser
 {
     IRParam*    nextParam;
     IRParam*    prevParam;
diff --git a/source/slang/slang.cpp b/source/slang/slang.cpp
index a918fa356..4238681f3 100644
--- a/source/slang/slang.cpp
+++ b/source/slang/slang.cpp
@@ -326,6 +326,23 @@ int CompileRequest::executeActionsInner()
     // Note that we *do* perform output generation as normal in pass-through mode.
     if (passThrough == PassThroughMode::None)
     {
+        // We currently allow GlSL files on the command line so that we can
+        // drive our "pass-through" mode, but we really want to issue an error
+        // message if the user is seriously asking us to compile them.
+        for (auto& translationUnit : translationUnits)
+        {
+            switch(translationUnit->sourceLanguage)
+            {
+            default:
+                break;
+
+            case SourceLanguage::GLSL:
+                mSink.diagnose(SourceLoc(), Diagnostics::glslIsNotSupported);
+                return 1;
+            }
+        }
+
+
         // Parse everything from the input files requested
         for (auto& translationUnit : translationUnits)
         {
diff --git a/tests/reflection/gh-55.glsl b/tests/reflection/gh-55.glsl
index f6db07482..49b6d2bcc 100644
--- a/tests/reflection/gh-55.glsl
+++ b/tests/reflection/gh-55.glsl
@@ -1,4 +1,8 @@
-//TEST(smoke):REFLECTION:-profile ps_4_0 -target glsl
+//TEST_DISABLED(smoke):REFLECTION:-profile ps_4_0 -target glsl
+
+// Disabled because we don't support GLSL any more.
+// (kept around so we can replace with an equivalent
+// test that uses HLSL input and tests GLSL layout rules)
 
 // Confirm fix for GitHub issue #55
 
diff --git a/tests/reflection/image-types.glsl b/tests/reflection/image-types.glsl
deleted file mode 100644
index 9bba69d3d..000000000
--- a/tests/reflection/image-types.glsl
+++ /dev/null
@@ -1,12 +0,0 @@
-//TEST(smoke):REFLECTION:-profile ps_4_0 -target glsl
-
-// Confirm that we expose GLSL `image` types through reflection
-
-layout(rgba32f)
-uniform writeonly imageBuffer iBuffer;
-
-layout(rgba32f)
-uniform writeonly image2D i2D;
-
-void main()
-{}
diff --git a/tests/reflection/image-types.glsl.expected b/tests/reflection/image-types.glsl.expected
deleted file mode 100644
index dfe477287..000000000
--- a/tests/reflection/image-types.glsl.expected
+++ /dev/null
@@ -1,33 +0,0 @@
-result code = 0
-standard error = {
-}
-standard output = {
-{
-    "parameters": [
-        {
-            "name": "iBuffer",
-            "binding": {"kind": "descriptorTableSlot", "index": 0},
-            "type": {
-                "kind": "resource",
-                "baseShape": "textureBuffer",
-                "access": "readWrite"
-            }
-        },
-        {
-            "name": "i2D",
-            "binding": {"kind": "descriptorTableSlot", "index": 1},
-            "type": {
-                "kind": "resource",
-                "baseShape": "texture2D",
-                "access": "readWrite"
-            }
-        }
-    ],
-    "entryPoints": [
-        {
-            "name": "main",
-            "stage:": "fragment"
-        }
-    ]
-}
-}
diff --git a/tests/reflection/std430-layout.glsl b/tests/reflection/std430-layout.glsl
index 73e48a8a0..d05c83785 100644
--- a/tests/reflection/std430-layout.glsl
+++ b/tests/reflection/std430-layout.glsl
@@ -1,5 +1,9 @@
 #version 450
-//TEST(smoke):REFLECTION:-profile ps_4_0 -target glsl
+//TEST_DISABLED(smoke):REFLECTION:-profile ps_4_0 -target glsl
+
+// Note: disabled because we don't support GLSL input any more
+// (kept around so we can replace with an equivalent
+// test that uses HLSL input and tests GLSL layout rules)
 
 // Confirm fix for GitHub issue #55