Change rules for layout of buffers/blocks containing only interface types (#1318)

TL;DR: This is a tweak the rules for layout that only affects a corner case for people who actually use `interface`-type shader parameters (which for now is just our own test cases). The tweaked rules seem like they make it easier to write the application code for interfacing with Slang, but even if we change our minds later the risk here should be low (again: nobody is using this stuff right now).

Slang already has a rule that a constant buffer that contains no ordinary/uniform data doesn't actually allocate a constant buffer `binding`/`register`:

    struct A { float4 x; Texture2D y; } // has uniform/ordinary data
    struct B { Texture2D u; SamplerState v; } // has none

    ConstantBuffer<A> gA; // gets a constant buffer register/binding
    ConstantBuffer<B> gB; // does not

There is similar logic for `ParameterBlock`, where the feature makes more sense. A user would be somewhat surprised if they declared a parmaeter block with a texture and a sampler in it, but then the generating code reserved Vulkan `binding=0` for a constant buffer they never asked for. The behavior in the case of a plain `ConstantBuffer` is chosen to be consistent with the parameter block case.

(Aside: all of this is a non-issue for targets with direct support for pointers, like CUDA and CPU. On those platforms a constant buffer or parameter block always translates to a pointer to the contained data.)

Now, suppose the user declares a constant buffer with an interface type in it:

    interface IFoo { ... }

    ConstantBuffer<IFoo> gBuffer;

When the layout logic sees the declaration of `gBuffer` it doesn't yet know what type will be plugged in as `IFoo` there. Will it contain uniform/ordinary data, such that a constant buffer is needed?

The existing logic in the type layout step implemented a complicated rule that amounted to:

* A `ConstantBuffer` or `cbuffer` that only contains `interface`/existential-type data will *not* be allocated a constant buffer `register`/`binding` during the initial layout process (on unspecialized code). That means that any resources declared after it will take the next consecutive `register`/`binding` without leaving any "gap" for the `ConstantBuffer` variable.

* After specialization (e.g., when we know that `Thing` should be plugged in for `IFoo`), if we discover that there is uniform/ordinary data in `Thing` then we will allocate a constant buffer `register`/`binding` for the `ConstantBuffer`, but that register/binding will necessarily come *after* any `register`s/`binding`s that were allocated to parameters during the first pass.

* Parameter blocks were intended to work the same when when it comes to whether or not they allocate a default `space`/`set`, but that logic appears to not have worked as intended.

These rules make some logical sense: a `ConstantBuffer` declaration only pays for what the element type actually needs, and if that changes due to specialization then the new resource allocation comes after the unspecialized resources (so that the locations of unspecialized parameters are stable across specializations).

The problem is that in practice it is almost impossible to write client application code that uses the Slang reflection API and makes reasonable choices in the presence of these rules. A general-purpose `ShaderObject` abstraction in application code ends up having to deal with multiple possible states that an object could be in:

1. An object where the element type `E` contains no uniform/ordinary data, and no interface/existential fields, so a constant buffer doesn't need to be allocated or bound.

2. An object where the element type `E` contains no uniform/ordinary data, but has interace/existential fields, with two sub-cases:

   a. When no values bound to interface/existential fields use uniform/ordinary dat, then the parent object must not bind a buffer

   b. When the type of value bound to an interface/existential field uses uniform/ordinary data, then the parent object needs to have a buffer allocated, and bind it.

3. When the element type `E` contains uniform/ordinary data, then a buffer should be allocated and bound (although its size/contents may change as interface/existential fields get re-bound)

Needing to deal with a possible shift between cases (2a) and (2b) based on what gets bound at runtime is a mess, and it is important to note that even though both (2a) and (3) require a buffer to be bound, the rules about *where* the buffer gets bound aren't consistent (so that the application needs to undrestand the distinction between "primary" and "pending" data in a type layout).

This change introduces a different rule, which seems to be more complicated to explain, but actually seems to simplify things for the application:

* A `ConstantBuffer` or `cbuffer` that only contains `interface`/existential-type data always has a constant buffer `register`/`binding` allocated for it "just in case."

* If after specialization there is any uniform/ordinary data, then that will use the buffer `register`/`binding` that was already allocated (that's easy enough).

* If after speciazliation there *isn't* any uniform/ordinary data, then the generated HLSL/GLSL shader code won't declare a buffer, but the `register`/`binding` is still claimed.

* A `ParameterBlock` behaves equivalently, so that if it contains any `interface`/existential fields, then it will always allocate a `space`/`set` "just in case"

The effect of these rules is to streamline the cases that an application needs to deal with down to two:

1. If the element type `E` of a shader object contains no uniform/ordinary or interface/existential fields, then no buffer needs to be allocated or bound

2. If the element type `E` contains *any* uniform/ordinary or interface/existential fields, then it is always safe to allocate and bind a buffer (even in the cases where it might be ignored).

Furthermore, the reflection data for the constant buffer `register`/`binding` becomes consistent in case (2), so that the application can always expect to find it in the same way.

3 files changed, 141 insertions, 102 deletions

diff --git a/source/slang/slang-type-layout.cpp b/source/slang/slang-type-layout.cpp
index f8c2bd377..bc6ed13a2 100644
--- a/source/slang/slang-type-layout.cpp
+++ b/source/slang/slang-type-layout.cpp
@@ -1797,6 +1797,11 @@ static bool _usesOrdinaryData(RefPtr<TypeLayout> typeLayout)
     return _usesResourceKind(typeLayout, LayoutResourceKind::Uniform);
 }
 
+static bool _usesExistentialData(RefPtr<TypeLayout> typeLayout)
+{
+    return _usesResourceKind(typeLayout, LayoutResourceKind::ExistentialObjectParam);
+}
+
     /// Add resource usage from `srcTypeLayout` to `dstTypeLayout` unless it would be "masked."
     ///
     /// This function is appropriate for applying resource usage from an element type
@@ -1918,9 +1923,32 @@ static RefPtr<TypeLayout> _createParameterGroupTypeLayout(
     //
     // To determine if we actually need a constant-buffer binding,
     // we will inspect the element type and see if it contains
-    // any ordinary/uniform data.
+    // any ordinary/uniform data *or* any interface/existential-type
+    // slots.
+    //
+    // The latter detail might sound surprising, because it means
+    // that for a declaration like:
     //
-    bool wantConstantBuffer = _usesOrdinaryData(rawElementTypeLayout);
+    //      cbuffer U { IThing gThing; }
+    //
+    // we will allocate a constant-buffer binding for `U` whether
+    // or not it turns out that the concrete type plugged in for
+    // `IThing gThing` has any ordinary/uniform data at all (that is,
+    // if the user plugs in a type that only holds a `Texture2D`,
+    // we will still have allocated the constant buffer binding/register,
+    // and waste it on an empty buffer).
+    //
+    // The reason for this choice is that it greatly simplifies
+    // logic for clients of Slang: a given `ConstantBuffer<>` or
+    // `cbuffer` variable can be statically determined to either
+    // need a constant buffer binding or not, based on its declared
+    // element type, and *nothing* that happens later can change
+    // that (e.g., plugging in a new value/object for `gThing`
+    // can't retroactively change whether or not `U` needed
+    // a constant buffer).
+    //
+    bool wantConstantBuffer = _usesOrdinaryData(rawElementTypeLayout)
+        || _usesExistentialData(rawElementTypeLayout);
     if( wantConstantBuffer )
     {
         // If there is any ordinary data, then we'll need to
@@ -1932,10 +1960,8 @@ static RefPtr<TypeLayout> _createParameterGroupTypeLayout(
     }
 
     // Similarly to how we only need a constant buffer to be allocated
-    // if the contents of the group actually had ordinary/uniform data,
-    // we also only want to allocate a `space` or `set` if that is really
-    // required.
-    //
+    // if the contents of the group actually call for it, we also only
+    // want to allocate a `space` or `set` if that is really required.
     //
     bool canUseSpaceOrSet = false;
     //
@@ -1970,9 +1996,11 @@ static RefPtr<TypeLayout> _createParameterGroupTypeLayout(
     if( canUseSpaceOrSet )
     {
         // Note that if we are allocating a constant buffer to hold
-        // some ordinary/uniform data then we definitely want a space/set,
-        // but we don't need to special-case that because the loop
-        // here will also detect the `LayoutResourceKind::Uniform` usage.
+        // some ordinary/uniform (or existential) data then we
+        // definitely want a space/set (because we will need it for
+        // the constant buffer we allocated above)  but we don't need
+        // to special-case that because the loop here will also detect
+        // the `LayoutResourceKind::Uniform` usage.
 
         for( auto elementResourceInfo : rawElementTypeLayout->resourceInfos )
         {
@@ -2080,89 +2108,27 @@ static RefPtr<TypeLayout> _createParameterGroupTypeLayout(
     {
         auto rules = rawElementTypeLayout->rules;
 
-        // One really annoying complication we need to deal with here
-        // its that it is possible that the original parameter group
-        // declaration didn't need a constant buffer or `space`/`set`
-        // to be allocated, but once we consider the "pending" data
-        // we need to have a constant buffer and/or space.
+        // Note that because we conservatively allocated both
+        // a constant buffer `register`/`binding` and a `space`/`set`
+        // for the container in cases where the element type
+        // might need it (which included interface/existential types),
+        // there is no need to worry about a case where `pendingElementType`
+        // could require a constant buffer `register`/`binding` or
+        // as `space`/`set` to be allocated but we didn't already
+        // allocate one in the non-pending layout.
+        //
+        // Out focus here is then on setting up the representation
+        // of the "pending" data for the element type, and in
+        // particular on dealing with any data that needs to
+        // "bleed through" to the resource usage of the overall
+        // parameter group.
         //
-        // We will compute whether the pending data create a demand
-        // for a constant buffer and/or a space/set, so that we know
-        // if we are in the tricky case.
-        //
-        bool pendingDataWantsConstantBuffer = _usesOrdinaryData(pendingElementTypeLayout);
-        bool pendingDataWantsSpaceOrSet = false;
-        if( canUseSpaceOrSet )
-        {
-            for( auto resInfo : pendingElementTypeLayout->resourceInfos )
-            {
-                if( resInfo.kind != LayoutResourceKind::RegisterSpace )
-                {
-                    pendingDataWantsSpaceOrSet = true;
-                    break;
-                }
-            }
-        }
-
-        // We will use a few different variables to track resource
-        // usage for the pending data, with roles similar to the
-        // umbrella type layout, container layout, and element layout
-        // that already came up for the main part of the parameter group type.
-
-
-        RefPtr<TypeLayout> pendingContainerTypeLayout = new TypeLayout();
-        pendingContainerTypeLayout->type = parameterGroupType;
-        pendingContainerTypeLayout->rules = parameterGroupRules;
-
-        containerTypeLayout->pendingDataTypeLayout = pendingContainerTypeLayout;
-
-        RefPtr<VarLayout> pendingContainerVarLayout = new VarLayout();
-        pendingContainerVarLayout->typeLayout = pendingContainerTypeLayout;
-
-        containerVarLayout->pendingVarLayout = pendingContainerVarLayout;
-
-
         RefPtr<VarLayout> pendingElementVarLayout = new VarLayout();
         pendingElementVarLayout->typeLayout = pendingElementTypeLayout;
 
         elementVarLayout->pendingVarLayout = pendingElementVarLayout;
 
-        // If we need a space/set for the pending data, and don't already
-        // have one, then we will allocate it now, as part of the
-        // "full" data type.
-        //
-        if( pendingDataWantsSpaceOrSet && !wantSpaceOrSet )
-        {
-            pendingContainerTypeLayout->addResourceUsage(LayoutResourceKind::RegisterSpace, 1);
-
-            // From here on, we know we have access to a register space,
-            // and we can mask any registers/bindings appropriately.
-            //
-            wantSpaceOrSet = true;
-        }
-
-        // If we need a constant buffer for laying out ordinary
-        // data, and didn't have one allocated before, we will create
-        // one.
-        //
-        if( pendingDataWantsConstantBuffer && !wantConstantBuffer )
-        {
-            auto cbUsage = rules->GetObjectLayout(ShaderParameterKind::ConstantBuffer);
-            pendingContainerTypeLayout->addResourceUsage(cbUsage.kind, cbUsage.size);
-
-            wantConstantBuffer = true;
-        }
-
-        for( auto resInfo : pendingContainerTypeLayout->resourceInfos )
-        {
-            pendingContainerVarLayout->findOrAddResourceInfo(resInfo.kind);
-        }
-
-        // Now that we've added in the resource usage for any CB or set/space
-        // we needed to allocate just for the pending data, we can safely
-        // lay out the pending data itself.
-        //
-        // The ordinary/uniform part of things wil always be "masked" and
+        // Any ordinary/uniform part of the pending data wil always be "masked" and
         // needs to come after any uniform data from the original element type.
         //
         // To kick things off we will initialize state for `struct` type layout,
@@ -2183,8 +2149,8 @@ static RefPtr<TypeLayout> _createParameterGroupTypeLayout(
             if( resInfo.kind == LayoutResourceKind::Uniform )
             {
                 // For the ordinary/uniform resource kind, we will add the resource
-                // usage as a structure field, and then write the resulting offset
-                // into the variable layout for the pending data.
+                // usage as if it was a structure field, and then write the resulting
+                // offset into the variable layout for the pending data.
                 //
                 auto offset = rules->AddStructField(
                     &uniformLayout,
@@ -2195,16 +2161,11 @@ static RefPtr<TypeLayout> _createParameterGroupTypeLayout(
             }
             else
             {
-                // For all other resource kinds, we will set the offset in
-                // the variable layout based on the total resources of that
-                // kind seen so far (including the "container" if any),
-                // and then bump the count for total resource usage.
+                // For all other resource kinds, we simply need to add an
+                // entry to the pending layout to represent the resource
+                // usage of the pending data.
                 //
-                auto elementVarResInfo = pendingElementVarLayout->findOrAddResourceInfo(resInfo.kind);
-                if( auto containerTypeInfo = pendingContainerTypeLayout->FindResourceInfo(resInfo.kind) )
-                {
-                    elementVarResInfo->index = containerTypeInfo->count.getFiniteValue();
-                }
+                pendingElementVarLayout->findOrAddResourceInfo(resInfo.kind);
             }
         }
         rules->EndStructLayout(&uniformLayout);
@@ -2218,7 +2179,6 @@ static RefPtr<TypeLayout> _createParameterGroupTypeLayout(
         // up the hierarchy.
         //
         RefPtr<TypeLayout> unmaskedPendingDataTypeLayout = new TypeLayout();
-        _addUnmaskedResourceUsage(true, unmaskedPendingDataTypeLayout, pendingContainerTypeLayout, wantSpaceOrSet);
         _addUnmaskedResourceUsage(false, unmaskedPendingDataTypeLayout, pendingElementTypeLayout, wantSpaceOrSet);
 
         // TODO: we should probably optimize for the case where there is no unmasked
@@ -2228,6 +2188,58 @@ static RefPtr<TypeLayout> _createParameterGroupTypeLayout(
         //
         typeLayout->pendingDataTypeLayout = unmaskedPendingDataTypeLayout;
 
+        // We will now attempt to compute reasonable offset information for
+        // (non-uniform) pending data in the element type. There are basically
+        // two cases here:
+        //
+        // 1. If the resource kind is one that is "masked" by the container,
+        // then the pending data can be statically placed at an offset fater
+        // the diret (non-pending) element data.
+        //
+        // 2. If the resource kind is one that "bleeds through" to the container,
+        // then its offset will always be relative to the location that
+        // gets allocated for pending data in the container, which means it
+        // is always zero.
+        //
+        // Because the offsets are currently all set to zero, we only
+        // need to check for case (1).
+        //
+        for( auto pendingVarResInfo : pendingElementVarLayout->resourceInfos )
+        {
+            auto kind = pendingVarResInfo.kind;
+
+            // If we are looking at uniform resource usage, we already
+            // handled it easlier.
+            //
+            if(kind == LayoutResourceKind::Uniform)
+                continue;
+
+            // If the usage is unmasked, the nwe are in case (2) and should
+            // skip out.
+            //
+            if(unmaskedPendingDataTypeLayout->FindResourceInfo(kind))
+                continue;
+
+            // Okay, we have resource info for somethign that is going
+            // to be "masked" by the container, in which case we
+            // can compute a fixed offset, after any existing data
+            // of the same kind.
+            //
+            auto existingVarResInfo = elementVarLayout->FindResourceInfo(kind);
+            if(!existingVarResInfo)
+                continue;
+
+            auto existingTypeResInfo = elementVarLayout->typeLayout->FindResourceInfo(kind);
+            if(!existingTypeResInfo)
+                continue;
+
+            // TODO: We need a more robust solution than just calling
+            // `getFiniteValue` here.
+            //
+            pendingVarResInfo.index = existingVarResInfo->index
+                + existingTypeResInfo->count.getFiniteValue();
+        }
+
         // TODO: we should probably adjust the size reported by the element type
         // to include any "pending" data that was allocated into the group, so
         // that it can be easier for client code to allocate their instances.
@@ -3025,7 +3037,7 @@ static TypeLayoutResult _createTypeLayout(
         //    buffer, including offsets, etc.
         //
         // 2. Compute information about any object types inside
-        //    the constant buffer, which need to be surfaces out
+        //    the constant buffer, which need to be surfaced out
         //    to the top level.
         //
         auto typeLayout = createParameterGroupTypeLayout(
diff --git a/tests/compute/interface-shader-param-in-struct.slang b/tests/compute/interface-shader-param-in-struct.slang
index a4dad6ccb..04854906a 100644
--- a/tests/compute/interface-shader-param-in-struct.slang
+++ b/tests/compute/interface-shader-param-in-struct.slang
@@ -44,20 +44,39 @@ int test(
 //TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):out
 RWStructuredBuffer<int> gOutputBuffer;
 
+// Note: even though `C` doesn't include any
+// uniform/ordinary dat as declared, it gets a
+// constant buffer allocated for it because
+// there is no way to rule out the possibility
+// that it *will* contain uniform/ordinary data
+// after specialization.
+//
+//TEST_INPUT:cbuffer(data=[0]):
 cbuffer C
 {
    IRandomNumberGenerationStrategy gStrategy;
 }
 
-//TEST_INPUT: globalExistentialType MyStrategy
-//TEST_INPUT:ubuffer(data=[1 2 4 8], stride=4):
-
 struct Stuff
 {
     IModifier modifier;
     int extra;
 }
 
+// Note: the data for global-scope existential parameters
+// is being introduced *before* the entry point declaration,
+// because the default policy used by `slangc` (which the
+// render test also uses) is to specialize the parameters at
+// the global scope (producing a new layout) and then compose
+// that specialized global scope with the entry point.
+//
+// (The net result is that data related to global-scope
+// specialization always precedes the data for entry point
+// parameters in these tests today)
+//
+//TEST_INPUT: globalExistentialType MyStrategy
+//TEST_INPUT:ubuffer(data=[1 2 4 8], stride=4):
+
 [numthreads(4, 1, 1)]
 void computeMain(
 //TEST_INPUT:cbuffer(data=[256]):
diff --git a/tests/compute/interface-shader-param4.slang b/tests/compute/interface-shader-param4.slang
index 317128613..625fc751c 100644
--- a/tests/compute/interface-shader-param4.slang
+++ b/tests/compute/interface-shader-param4.slang
@@ -46,6 +46,14 @@ int test(
 //TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):out
 RWStructuredBuffer<int> gOutputBuffer;
 
+// Note: a constant buffer register/binding is always claimed
+// for `gStrategy` during initial compilation (before specialization)
+// because the layout logic has no way of knowing if the type
+// that gets plugged in will involve uniform/ordinary data
+// or not.
+//
+//TEST_INPUT:cbuffer(data=[0]):
+//
 ConstantBuffer<IRandomNumberGenerationStrategy> gStrategy;
 
 // The concrete types we plug in for `gStrategy` and `modifier`