More fixups for parameter block binding generation (#311)

* More fixups for parameter block binding generation

The bug in this case arises when there is both a parameter block and global-scope resources, all of which are relying on automatic binding assignment. If the parameter block is the first global-scope parameter that gets encountered, then it is possible for it to allocate regsiter space/set zero for itself, which confuses the logic for handling other global-scope parameters (which assumes that *they* get space/set zero).

I've also made some fixup to the reflection test harness and reflection API code:

- Have the hardness handle register-space allocations when printing, and be sure to only show their `index` and not their `space` (since that would be redundant)

- Have the reflection API only auto-redirect queries on a parameter group type layout to its container type layout *if* the container type layout has a non-zero number of resource allocations. The problem that arises here is a `ParameterBlock<X>` where `X` doesn't contain any uniforms, so that no container is needed. In that case the container ends up with no resource allocation(s).

* Fixups for test failures.

- The thread-group size tests failed because they had shader parameters with no resources to back them (built-in `SV_` inputs), and the printing of those changed. I fixed up the baseline, but also had to fix a few bugs in the reflection test fixture's printing logic.

- The GLSL parameter block test revealed a corner case of the existing logic: because we always need to generate a binding for the "hack" sampler (even if code doesn't end up needing it), and that sampler should always go in the "default" set (should be set zero), the user's `ParameterBlock` will always end up as `set=1` or later, even if there are no other global-scope parameters.

  - This will be fixed once we don't have to rely on glslang's annoying behavior in this one case, either because glslang gets fixed, or because we implement our own SPIR-V codegen.

8 files changed, 260 insertions, 68 deletions

diff --git a/source/slang/parameter-binding.cpp b/source/slang/parameter-binding.cpp
index 66f7d437c..4eb1cf6ee 100644
--- a/source/slang/parameter-binding.cpp
+++ b/source/slang/parameter-binding.cpp
@@ -99,6 +99,22 @@ struct UsedRanges
         return Add(range);
     }
 
+    bool contains(UInt index)
+    {
+        for (auto rr : ranges)
+        {
+            if (index < rr.begin)
+                return false;
+
+            if (index >= rr.end)
+                continue;
+
+            return true;
+        }
+
+        return false;
+    }
+
 
     // Try to find space for `count` entries
     UInt Allocate(ParameterInfo* param, UInt count)
@@ -216,6 +232,9 @@ struct SharedParameterBindingContext
     // Which register spaces have been claimed so far?
     UsedRanges usedSpaces;
 
+    // The space to use for auto-generated bindings.
+    UInt defaultSpace = 0;
+
     TargetRequest* getTargetRequest() { return targetRequest; }
 };
 
@@ -1058,12 +1077,30 @@ static void completeBindingsForParameter(
             continue;
         }
 
-        // For now we only auto-generate bindings in space zero
+        // Auto-generated bindings will all go in the same space,
+        // which was allocated up front.
         //
-        // TODO: we may want to support searching for a space with
-        // capacity for our resource, just in case somebody has
-        // claimed the entire range...
-        UInt space = 0;
+        // We don't currently worry about running out of room in
+        // this space; if the user declares enough parameters
+        // to overflow the range then we will have other problems
+        // on our hands.
+        //
+        // The one case that might seem like a challenge is unsized
+        // arrays, since these conceptually require a (countably)
+        // infinite register range.
+        //
+        // This turns out not to be a problem that this code
+        // needs to handle, for two reasons:
+        //
+        // 1) In the D3D case, an unbounded-size array should be
+        // computed to require one (or more) whole register spaces,
+        // and so we'd end up in the `RegisterSpace` case above.
+        //
+        // 2) In the Vulkan case, an unbounded-size array of
+        // resources still uses only a single binding, so we
+        // won't run out of space.
+        //
+        UInt space = context->shared->defaultSpace;
 
         RefPtr<UsedRangeSet> usedRangeSet;
         switch (kind)
@@ -1737,7 +1774,12 @@ void generateParameterBindings(
         generateParameterBindings(&context, parameter);
     }
 
-    bool anyGlobalUniforms = false;
+    // Determine if there are any global-scope parameters that use `Uniform`
+    // resources, and thus need to get packaged into a constant buffer.
+    //
+    // Note: this doesn't account for GLSL's support for "legacy" uniforms
+    // at global scope, which don't get assigned a CB.
+    bool needDefaultConstantBuffer = false;
     for( auto& parameterInfo : sharedContext.parameters )
     {
         SLANG_RELEASE_ASSERT(parameterInfo->varLayouts.Count() != 0);
@@ -1746,28 +1788,104 @@ void generateParameterBindings(
         // Does the field have any uniform data?
         if( firstVarLayout->typeLayout->FindResourceInfo(LayoutResourceKind::Uniform) )
         {
-            anyGlobalUniforms = true;
+            needDefaultConstantBuffer = true;
             break;
         }
     }
 
+    // Next, we want to determine if there are any global-scope parameters
+    // that don't just allocate a whole register space to themselves; these
+    // parameters will need to go into a "default" space, which should always
+    // be the first space we allocate.
+    //
+    // As a starting point, we will definitely need a "default" space if
+    // we are creating a default constant buffer, since it should get
+    // a binding in that "default" space.
+    bool needDefaultSpace = needDefaultConstantBuffer;
+    if (!needDefaultSpace)
+    {
+        for (auto& parameterInfo : sharedContext.parameters)
+        {
+            SLANG_RELEASE_ASSERT(parameterInfo->varLayouts.Count() != 0);
+            auto firstVarLayout = parameterInfo->varLayouts.First();
+
+            // Does the parameter have any resource usage that isn't just
+            // allocating a whole register space?
+            for (auto resInfo : firstVarLayout->typeLayout->resourceInfos)
+            {
+                if (resInfo.kind != LayoutResourceKind::RegisterSpace)
+                {
+                    needDefaultSpace = true;
+                    break;
+                }
+            }
+        }
+    }
+    // If we are having to insert our "hack" default sampler, then
+    // we need to put it in the default space.
+    if (isGLSLCrossCompilerNeeded(targetReq))
+    {
+        needDefaultSpace = true;
+    }
+
+    // If we need a space for default bindings, then allocate it here.
+    if (needDefaultSpace)
+    {
+        UInt defaultSpace = 0;
+
+        // Check if space #0 has been allocated yet. If not, then we'll
+        // want to use it.
+        if (sharedContext.usedSpaces.contains(0))
+        {
+            // Somebody has already put things in space zero.
+            //
+            // TODO: There are two cases to handle here:
+            //
+            // 1) If there is any free register ranges in space #0,
+            // then we should keep using it as the default space.
+            //
+            // 2) If somebody went and put an HLSL unsized array into space #0,
+            // *or* if they manually placed something like a paramter block
+            // there (which should consume whole spaces), then we need to
+            // allocate an unused space instead.
+            //
+            // For now we don't deal with the concept of unsized arrays, or
+            // manually assigning parameter blocks to spaces, so we punt
+            // on this and assume case (1).
+
+            defaultSpace = 0;
+        }
+        else
+        {
+            // Nobody has used space zero yet, so we need
+            // to make sure to reserve it for defaults.
+            defaultSpace = allocateUnusedSpaces(&context, 1);
+
+            // The result of this allocation had better be that
+            // we got space #0, or else something has gone wrong.
+            SLANG_ASSERT(defaultSpace == 0);
+        }
+
+        sharedContext.defaultSpace = defaultSpace;
+    }
+
     // If there are any global-scope uniforms, then we need to
     // allocate a constant-buffer binding for them here.
     ParameterBindingInfo globalConstantBufferBinding;
     globalConstantBufferBinding.index = 0;
-    if( anyGlobalUniforms )
+    globalConstantBufferBinding.space = 0;
+    if( needDefaultConstantBuffer )
     {
         // TODO: this logic is only correct for D3D targets, where
         // global-scope uniforms get wrapped into a constant buffer.
 
-        UInt space = 0;
+        UInt space = sharedContext.defaultSpace;
         auto usedRangeSet = findUsedRangeSetForSpace(&context, space);
 
         globalConstantBufferBinding.index =
             usedRangeSet->usedResourceRanges[
                 (int)LayoutResourceKind::ConstantBuffer].Allocate(nullptr, 1);
 
-        // For now we only auto-generate bindings in space zero
         globalConstantBufferBinding.space = space;
     }
 
@@ -1839,7 +1957,7 @@ void generateParameterBindings(
 
     // If there are global-scope uniforms, then we need to wrap
     // up a global constant buffer type layout to hold them
-    if( anyGlobalUniforms )
+    if( needDefaultConstantBuffer )
     {
         auto globalConstantBufferLayout = createParameterGroupTypeLayout(
             layoutContext,
@@ -1857,7 +1975,7 @@ void generateParameterBindings(
     // being invoked, so that we don't gum up other shaders.
     if(isGLSLCrossCompilerNeeded(targetReq))
     {
-        UInt space = 0;
+        UInt space = sharedContext.defaultSpace;
         auto hackSamplerUsedRanges = findUsedRangeSetForSpace(&context, space);
 
         UInt binding = hackSamplerUsedRanges->usedResourceRanges[(int)LayoutResourceKind::DescriptorTableSlot].Allocate(nullptr, 1);
@@ -1888,10 +2006,10 @@ void generateParameterBindings(
     // record into a suitable object that represents the program
     RefPtr<VarLayout> globalVarLayout = new VarLayout();
     globalVarLayout->typeLayout = globalScopeLayout;
-    if (anyGlobalUniforms)
+    if (needDefaultConstantBuffer)
     {
         auto cbInfo = globalVarLayout->findOrAddResourceInfo(LayoutResourceKind::ConstantBuffer);
-        cbInfo->space = 0;
+        cbInfo->space = globalConstantBufferBinding.space;
         cbInfo->index = globalConstantBufferBinding.index;
     }
     programLayout->globalScopeLayout = globalVarLayout;
diff --git a/source/slang/reflection.cpp b/source/slang/reflection.cpp
index cc2c6b289..bd95f48fa 100644
--- a/source/slang/reflection.cpp
+++ b/source/slang/reflection.cpp
@@ -574,15 +574,26 @@ static SlangParameterCategory getParameterCategory(
     return SLANG_PARAMETER_CATEGORY_MIXED;
 }
 
+static TypeLayout* maybeGetContainerLayout(TypeLayout* typeLayout)
+{
+    if (auto parameterGroupTypeLayout = dynamic_cast<ParameterGroupTypeLayout*>(typeLayout))
+    {
+        auto containerTypeLayout = parameterGroupTypeLayout->containerVarLayout->typeLayout;
+        if (containerTypeLayout->resourceInfos.Count() != 0)
+        {
+            return containerTypeLayout;
+        }
+    }
+
+    return typeLayout;
+}
+
 SLANG_API SlangParameterCategory spReflectionTypeLayout_GetParameterCategory(SlangReflectionTypeLayout* inTypeLayout)
 {
     auto typeLayout = convert(inTypeLayout);
     if(!typeLayout) return SLANG_PARAMETER_CATEGORY_NONE;
 
-    if (auto parameterGroupTypeLayout = dynamic_cast<ParameterGroupTypeLayout*>(typeLayout))
-    {
-        typeLayout = parameterGroupTypeLayout->containerVarLayout->typeLayout;
-    }
+    typeLayout = maybeGetContainerLayout(typeLayout);
 
     return getParameterCategory(typeLayout);
 }
@@ -592,10 +603,7 @@ SLANG_API unsigned spReflectionTypeLayout_GetCategoryCount(SlangReflectionTypeLa
     auto typeLayout = convert(inTypeLayout);
     if(!typeLayout) return 0;
 
-    if (auto parameterGroupTypeLayout = dynamic_cast<ParameterGroupTypeLayout*>(typeLayout))
-    {
-        typeLayout = parameterGroupTypeLayout->containerVarLayout->typeLayout;
-    }
+    typeLayout = maybeGetContainerLayout(typeLayout);
 
     return (unsigned) typeLayout->resourceInfos.Count();
 }
@@ -605,10 +613,7 @@ SLANG_API SlangParameterCategory spReflectionTypeLayout_GetCategoryByIndex(Slang
     auto typeLayout = convert(inTypeLayout);
     if(!typeLayout) return SLANG_PARAMETER_CATEGORY_NONE;
 
-    if (auto parameterGroupTypeLayout = dynamic_cast<ParameterGroupTypeLayout*>(typeLayout))
-    {
-        typeLayout = parameterGroupTypeLayout->containerVarLayout->typeLayout;
-    }
+    typeLayout = maybeGetContainerLayout(typeLayout);
 
     return typeLayout->resourceInfos[index].kind;
 }
diff --git a/tests/bindings/glsl-parameter-blocks.slang.glsl b/tests/bindings/glsl-parameter-blocks.slang.glsl
index 5094debb1..9956800f1 100644
--- a/tests/bindings/glsl-parameter-blocks.slang.glsl
+++ b/tests/bindings/glsl-parameter-blocks.slang.glsl
@@ -6,16 +6,16 @@ struct Test
     vec4 a;
 };
 
-layout(binding = 0)
+layout(binding = 0, set = 1)
 uniform gTest_S1
 {
     Test gTest;
 };
 
-layout(binding = 1)
+layout(binding = 1, set = 1)
 uniform texture2D gTest_t;
 
-layout(binding = 2)
+layout(binding = 2, set = 1)
 uniform sampler gTest_s;
 
 vec4 main_(vec2 uv)
diff --git a/tests/reflection/parameter-block.slang b/tests/reflection/parameter-block.slang
new file mode 100644
index 000000000..c20337480
--- /dev/null
+++ b/tests/reflection/parameter-block.slang
@@ -0,0 +1,19 @@
+//TEST:REFLECTION:-profile glsl_fragment -target glsl
+
+// Confirm that we do parameter binding correctly
+// when we have both a parameter block *and* user-defined
+// resource parameters that both need automatic
+// binding allocation.
+
+struct Helper
+{
+	Texture2D t;
+	SamplerState s;	
+};
+
+ParameterBlock<Helper> a;
+
+Texture2D b;
+
+float4 main() : SV_target
+{ return 0.0; }
diff --git a/tests/reflection/parameter-block.slang.expected b/tests/reflection/parameter-block.slang.expected
new file mode 100644
index 000000000..bc04d79ed
--- /dev/null
+++ b/tests/reflection/parameter-block.slang.expected
@@ -0,0 +1,52 @@
+result code = 0
+standard error = {
+}
+standard output = {
+{
+    "parameters": [
+        {
+            "name": "a",
+            "binding": {"kind": "registerSpace", "index": 1},
+            "type": {
+                "kind": "parameterBlock",
+                "elementType": {
+                    "kind": "struct",
+                    "name": "Helper",
+                    "fields": [
+                        {
+                            "name": "t",
+                            "type": {
+                                "kind": "resource",
+                                "baseShape": "texture2D"
+                            },
+                            "binding": {"kind": "descriptorTableSlot", "index": 0}
+                        },
+                        {
+                            "name": "s",
+                            "type": {
+                                "kind": "samplerState"
+                            },
+                            "binding": {"kind": "descriptorTableSlot", "index": 1}
+                        }
+                    ]
+                }
+            }
+        },
+        {
+            "name": "b",
+            "binding": {"kind": "descriptorTableSlot", "index": 0},
+            "type": {
+                "kind": "resource",
+                "baseShape": "texture2D"
+            }
+        },
+        {
+            "name": "SLANG_hack_samplerForTexelFetch",
+            "binding": {"kind": "descriptorTableSlot", "index": 1},
+            "type": {
+                "kind": "samplerState"
+            }
+        }
+    ]
+}
+}
diff --git a/tests/reflection/thread-group-size.comp.expected b/tests/reflection/thread-group-size.comp.expected
index ea7e97851..facd52cc0 100644
--- a/tests/reflection/thread-group-size.comp.expected
+++ b/tests/reflection/thread-group-size.comp.expected
@@ -21,10 +21,7 @@ standard output = {
                                     "kind": "scalar",
                                     "scalarType": "float32"
                                 }
-                            },
-                            "bindings": [
-
-                            ]
+                            }
                         }
                     ]
                 }
diff --git a/tests/reflection/thread-group-size.hlsl.expected b/tests/reflection/thread-group-size.hlsl.expected
index a41c66248..cd5d09e35 100644
--- a/tests/reflection/thread-group-size.hlsl.expected
+++ b/tests/reflection/thread-group-size.hlsl.expected
@@ -21,9 +21,6 @@ standard output = {
             "parameters": [
                 {
                     "name": "tid",
-                    "bindings": [
-
-                    ],
                     "semanticName": "SV_DISPATCHTHREADID",
                     "type": {
                         "kind": "vector",
diff --git a/tools/slang-reflection-test/main.cpp b/tools/slang-reflection-test/main.cpp
index 057bcd127..2b5477b4a 100644
--- a/tools/slang-reflection-test/main.cpp
+++ b/tools/slang-reflection-test/main.cpp
@@ -116,6 +116,7 @@ static void emitReflectionVarBindingInfoJSON(
     CASE(DESCRIPTOR_TABLE_SLOT, descriptorTableSlot);
     CASE(SPECIALIZATION_CONSTANT, specializationConstant);
     CASE(MIXED, mixed);
+    CASE(REGISTER_SPACE, registerSpace);
     #undef CASE
 
         default:
@@ -124,7 +125,7 @@ static void emitReflectionVarBindingInfoJSON(
             break;
         }
         write(writer, "\"");
-        if( space )
+        if( space && category != SLANG_PARAMETER_CATEGORY_REGISTER_SPACE)
         {
             write(writer, ", ");
             write(writer, "\"space\": ");
@@ -149,6 +150,7 @@ static void emitReflectionVarBindingInfoJSON(
     auto stage = var->getStage();
     if (stage != SLANG_STAGE_NONE)
     {
+        write(writer, ",\n");
         char const* stageName = "UNKNOWN";
         switch (stage)
         {
@@ -165,45 +167,49 @@ static void emitReflectionVarBindingInfoJSON(
 
         write(writer, "\"stage\": \"");
         write(writer, stageName);
-        write(writer, "\",\n");
+        write(writer, "\"");
     }
 
     auto typeLayout = var->getTypeLayout();
     auto categoryCount = var->getCategoryCount();
 
-    if( categoryCount != 1 )
+    if (categoryCount)
     {
-        write(writer,"\"bindings\": [\n");
-    }
-    else
-    {
-        write(writer,"\"binding\": ");
-    }
-    indent(writer);
+        write(writer, ",\n");
+        if( categoryCount != 1 )
+        {
+            write(writer,"\"bindings\": [\n");
+        }
+        else
+        {
+            write(writer,"\"binding\": ");
+        }
+        indent(writer);
 
-    for(uint32_t cc = 0; cc < categoryCount; ++cc )
-    {
-        auto category = var->getCategoryByIndex(cc);
-        auto index = var->getOffset(category);
-        auto space = var->getBindingSpace(category);
-        auto count = typeLayout->getSize(category);
+        for(uint32_t cc = 0; cc < categoryCount; ++cc )
+        {
+            auto category = var->getCategoryByIndex(cc);
+            auto index = var->getOffset(category);
+            auto space = var->getBindingSpace(category);
+            auto count = typeLayout->getSize(category);
 
-        if (cc != 0) write(writer, ",\n");
+            if (cc != 0) write(writer, ",\n");
 
-        write(writer,"{");
-        emitReflectionVarBindingInfoJSON(
-            writer,
-            category,
-            index,
-            count,
-            space);
-        write(writer,"}");
-    }
+            write(writer,"{");
+            emitReflectionVarBindingInfoJSON(
+                writer,
+                category,
+                index,
+                count,
+                space);
+            write(writer,"}");
+        }
 
-    dedent(writer);
-    if( categoryCount != 1 )
-    {
-        write(writer,"\n]");
+        dedent(writer);
+        if( categoryCount != 1 )
+        {
+            write(writer,"\n]");
+        }
     }
 
     if (auto semanticName = var->getSemanticName())
@@ -244,7 +250,6 @@ static void emitReflectionVarLayoutJSON(
 
     write(writer, "\"type\": ");
     emitReflectionTypeLayoutJSON(writer, var->getTypeLayout());
-    write(writer, ",\n");
 
     emitReflectionVarBindingInfoJSON(writer, var);
 
@@ -596,7 +601,6 @@ static void emitReflectionParamJSON(
     indent(writer);
 
     emitReflectionNameInfoJSON(writer, param->getName());
-    write(writer, ",\n");
 
     emitReflectionVarBindingInfoJSON(writer, param);
     write(writer, ",\n");