Allow parameter blocks to be explicitly bound to spaces (#736)

* Don't look at VK bindings when compiling for D3D and vice versa

The compiler had been looking at all the modifiers on a declaration when piecing together binding information, whether or not those modifiers should apply on the chosen target API. This was working in practice because the "layout resource kinds" used by each API target were disjoint, for the most part.

This change ensures that we don't even look at modifiers that don't apply on the chosen target, and furthermore adds a new warning that applies if the user is compiling a shader with explicit `register` bindings for Vulkan, if there are no corresponding `[[vk::binding(...)]]` attributes (under the assumption that if they want to be explicit in one case, they probably want to be explicit in all cases).

* Allow explicit space/set bindings on parameter blocks

The syntax for the D3D case is to specify a `space` in a `register` modifier, without any other register class:

```hlsl
ParameterBlock<X> myBlock : regsiter(space999);
```

In the Vulkan case, the user must apply the `[[vk::binding(...)]]` attribute and is expected to use a `binding` of zero:

```hlsl
[[vk::binding(0,999)]]
ParameterBlock<X> myBlock;
```

This change includes a reflection test for the new capability (where we also confirm that it produces the expected output when compared with fxc), and a test for the diagnostic messages when the user messes up bindings for Vulkan.

The implementation itself is fairly straightforward, since the compiler already treats registe spaces/sets as a resource that parameters can consume directly.

Note: the test case for explicit parameter block space/set bindings includes some commented out code that lead to a compiler crash. I would like to fix the underlying issue, but it seemed sensible to keep the bug fix out of a change like this that is adding functionality.

4 files changed, 129 insertions, 43 deletions

diff --git a/source/slang/compiler.h b/source/slang/compiler.h
index 7be5b332d..fbd6b3f15 100644
--- a/source/slang/compiler.h
+++ b/source/slang/compiler.h
@@ -241,6 +241,12 @@ namespace Slang
         MatrixLayoutMode getDefaultMatrixLayoutMode();
     };
 
+        /// Are we generating code for a D3D API?
+    bool isD3DTarget(TargetRequest* targetReq);
+
+        /// Are we generating code for a Khronos API (OpenGL or Vulkan)?
+    bool isKhronosTarget(TargetRequest* targetReq);
+
     // Compute the "effective" profile to use when outputting the given entry point
     // for the chosen code-generation target.
     //
diff --git a/source/slang/diagnostic-defs.h b/source/slang/diagnostic-defs.h
index 8466485e7..2a08dfb6a 100644
--- a/source/slang/diagnostic-defs.h
+++ b/source/slang/diagnostic-defs.h
@@ -368,6 +368,9 @@ DIAGNOSTIC(39009, Error, expectedSpace, "expected 'space', got '$0'")
 DIAGNOSTIC(39010, Error, expectedSpaceIndex, "expected a register space index after 'space'")
 DIAGNOSTIC(39011, Error, componentMaskNotSupported, "explicit register component masks are not yet supported in Slang")
 DIAGNOSTIC(39012, Error, packOffsetNotSupported, "explicit 'packoffset' bindings are not yet supported in Slang")
+DIAGNOSTIC(39013, Warning, registerModifierButNoVulkanLayout, "shader parameter '$0' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan")
+DIAGNOSTIC(39014, Error, unexpectedSpecifierAfterSpace, "unexpected specifier after register space: '$0'")
+DIAGNOSTIC(39015, Error, wholeSpaceParameterRequiresZeroBinding, "shader parameter '$0' consumes whole descriptor sets, so the binding must be in the form '[[vk::binding(0, ...)]]'; the non-zero binding '$1' is not allowed")
 
 DIAGNOSTIC(39013, Error, dontExpectOutParametersForStage, "the '$0' stage does not support `out` or `inout` entry point parameters")
 DIAGNOSTIC(39014, Error, dontExpectInParametersForStage, "the '$0' stage does not support `in` entry point parameters")
diff --git a/source/slang/parameter-binding.cpp b/source/slang/parameter-binding.cpp
index bc0f5b760..58891a956 100644
--- a/source/slang/parameter-binding.cpp
+++ b/source/slang/parameter-binding.cpp
@@ -338,7 +338,7 @@ struct SharedParameterBindingContext
     // TODO: We should eventually strip this down to
     // just the subset of fields on the target that
     // can influence layout decisions.
-    TargetRequest*  targetRequest;
+    TargetRequest*  targetRequest = nullptr;
 
     LayoutRulesFamilyImpl* defaultLayoutRules;
 
@@ -443,6 +443,36 @@ static void splitNameAndIndex(
     outDigits = UnownedStringSlice(digitsBegin, digitsEnd);
 }
 
+LayoutResourceKind findRegisterClassFromName(UnownedStringSlice const& registerClassName)
+{
+    switch( registerClassName.size() )
+    {
+    case 1:
+        switch (*registerClassName.begin())
+        {
+        case 'b': return LayoutResourceKind::ConstantBuffer;
+        case 't': return LayoutResourceKind::ShaderResource;
+        case 'u': return LayoutResourceKind::UnorderedAccess;
+        case 's': return LayoutResourceKind::SamplerState;
+
+        default:
+            break;
+        }
+        break;
+
+    case 5:
+        if( registerClassName == "space" )
+        {
+            return LayoutResourceKind::RegisterSpace;
+        }
+        break;
+
+    default:
+        break;
+    }
+    return LayoutResourceKind::None;
+}
+
 LayoutSemanticInfo ExtractLayoutSemanticInfo(
     ParameterBindingContext*    context,
     HLSLLayoutSemantic*         semantic)
@@ -470,31 +500,9 @@ LayoutSemanticInfo ExtractLayoutSemanticInfo(
     UnownedStringSlice registerIndexDigits;
     splitNameAndIndex(registerName, registerClassName, registerIndexDigits);
 
-    // All of the register classes we support are single ASCII characters,
-    // so we really just care about the first byte, but we want to be
-    // careful and only look at it if the register class name is one
-    // byte long.
-    char registerClassChar = registerClassName.size() == 1 ? *registerClassName.begin() : 0;
-    LayoutResourceKind kind = LayoutResourceKind::None;
-    switch (registerClassChar)
+    LayoutResourceKind kind = findRegisterClassFromName(registerClassName);
+    if(kind == LayoutResourceKind::None)
     {
-    case 'b':
-        kind = LayoutResourceKind::ConstantBuffer;
-        break;
-
-    case 't':
-        kind = LayoutResourceKind::ShaderResource;
-        break;
-
-    case 'u':
-        kind = LayoutResourceKind::UnorderedAccess;
-        break;
-
-    case 's':
-        kind = LayoutResourceKind::SamplerState;
-        break;
-
-    default:
         getSink(context)->diagnose(semantic->registerName, Diagnostics::unknownRegisterClass, registerClassName);
         return info;
     }
@@ -524,13 +532,17 @@ LayoutSemanticInfo ExtractLayoutSemanticInfo(
             UnownedStringSlice spaceDigits;
             splitNameAndIndex(spaceName, spaceSpelling, spaceDigits);
 
-            if( spaceSpelling != UnownedTerminatedStringSlice("space") )
+            if( kind == LayoutResourceKind::RegisterSpace )
+            {
+                getSink(context)->diagnose(registerSemantic->spaceName, Diagnostics::unexpectedSpecifierAfterSpace, spaceName);
+            }
+            else if( spaceSpelling != UnownedTerminatedStringSlice("space") )
             {
-                getSink(context)->diagnose(semantic->registerName, Diagnostics::expectedSpace, spaceSpelling);
+                getSink(context)->diagnose(registerSemantic->spaceName, Diagnostics::expectedSpace, spaceSpelling);
             }
             else if( spaceDigits.size() == 0 )
             {
-                getSink(context)->diagnose(semantic->registerName, Diagnostics::expectedSpaceIndex);
+                getSink(context)->diagnose(registerSemantic->spaceName, Diagnostics::expectedSpaceIndex);
             }
             else
             {
@@ -1491,6 +1503,26 @@ static void addExplicitParameterBindings_HLSL(
     RefPtr<ParameterInfo>       parameterInfo,
     RefPtr<VarLayout>           varLayout)
 {
+    // We only want to apply D3D `register` modifiers when compiling for
+    // D3D targets.
+    //
+    // TODO: Nominally, the `register` keyword allows for a shader
+    // profile to be specified, so that a given binding only
+    // applies for a specific profile:
+    //
+    //      https://docs.microsoft.com/en-us/windows/desktop/direct3dhlsl/dx-graphics-hlsl-variable-register
+    //
+    // We might want to consider supporting that syntax in the
+    // long run, in order to handle bindings for multiple targets
+    // in a more consistent fashion (whereas using `register` for D3D
+    // and `[[vk::binding(...)]]` for Vulkan creates a lot of
+    // visual noise).
+    //
+    // For now we do the filtering on target in a very direct fashion:
+    //
+    if(!isD3DTarget(context->getTargetRequest()))
+        return;
+
     auto typeLayout = varLayout->typeLayout;
     auto varDecl = varLayout->varDecl;
 
@@ -1527,11 +1559,35 @@ static void addExplicitParameterBindings_HLSL(
     }
 }
 
+static void maybeDiagnoseMissingVulkanLayoutModifier(
+    ParameterBindingContext*    context,
+    DeclRef<VarDeclBase> const& varDecl)
+{
+    // If the user didn't specify a `binding` (and optional `set`) for Vulkan,
+    // but they *did* specify a `register` for D3D, then that is probably an
+    // oversight on their part.
+    if( auto registerModifier = varDecl.getDecl()->FindModifier<HLSLRegisterSemantic>() )
+    {
+        getSink(context)->diagnose(registerModifier, Diagnostics::registerModifierButNoVulkanLayout, varDecl.GetName());
+    }
+}
+
 static void addExplicitParameterBindings_GLSL(
     ParameterBindingContext*    context,
     RefPtr<ParameterInfo>       parameterInfo,
     RefPtr<VarLayout>           varLayout)
 {
+
+    // We only want to apply GLSL-style layout modifers
+    // when compiling for a Khronos-related target.
+    //
+    // TODO: This should have some finer granularity
+    // so that we are able to distinguish between
+    // Vulkan and OpenGL as targets.
+    //
+    if(!isKhronosTarget(context->getTargetRequest()))
+        return;
+
     auto typeLayout = varLayout->typeLayout;
     auto varDecl = varLayout->varDecl;
 
@@ -1556,11 +1612,28 @@ static void addExplicitParameterBindings_GLSL(
         auto attr = varDecl.getDecl()->FindModifier<GLSLBindingAttribute>();
         if (!attr)
         {
+            maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl);
             return;
         }
         semanticInfo.index = attr->binding;
         semanticInfo.space = attr->set;
     }
+    else if( (resInfo = typeLayout->FindResourceInfo(LayoutResourceKind::RegisterSpace)) != nullptr )
+    {
+        // Try to find `set`
+        auto attr = varDecl.getDecl()->FindModifier<GLSLBindingAttribute>();
+        if (!attr)
+        {
+            maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl);
+            return;
+        }
+        if( attr->binding != 0)
+        {
+            getSink(context)->diagnose(attr, Diagnostics::wholeSpaceParameterRequiresZeroBinding, varDecl.GetName(), attr->binding);
+        }
+        semanticInfo.index = attr->set;
+        semanticInfo.space = 0;
+    }
     else if( (resInfo = typeLayout->FindResourceInfo(LayoutResourceKind::VertexInput)) != nullptr )
     {
         // Try to find `location` binding
@@ -2564,6 +2637,7 @@ void generateParameterBindings(
     sharedContext.compileRequest = compileReq;
     sharedContext.defaultLayoutRules = layoutContext.getRulesFamily();
     sharedContext.programLayout = programLayout;
+    sharedContext.targetRequest = targetReq;
 
     // Create a sub-context to collect parameters that get
     // declared into the global scope
@@ -2816,6 +2890,7 @@ RefPtr<ProgramLayout> specializeProgramLayout(
     sharedContext.compileRequest = targetReq->compileRequest;
     sharedContext.defaultLayoutRules = layoutContext.getRulesFamily();
     sharedContext.programLayout = newProgramLayout;
+    sharedContext.targetRequest = targetReq;
 
     // Create a sub-context to collect parameters that get
     // declared into the global scope
diff --git a/source/slang/type-layout.cpp b/source/slang/type-layout.cpp
index 41476a7e5..4e9a05b53 100644
--- a/source/slang/type-layout.cpp
+++ b/source/slang/type-layout.cpp
@@ -823,7 +823,7 @@ static bool isOpenGLTarget(TargetRequest*)
     return false;
 }
 
-static bool isD3DTarget(TargetRequest* targetReq)
+bool isD3DTarget(TargetRequest* targetReq)
 {
     switch( targetReq->target )
     {
@@ -839,6 +839,20 @@ static bool isD3DTarget(TargetRequest* targetReq)
     }
 }
 
+bool isKhronosTarget(TargetRequest* targetReq)
+{
+    switch( targetReq->target )
+    {
+    default:
+        return false;
+
+    case CodeGenTarget::GLSL:
+    case CodeGenTarget::SPIRV:
+    case CodeGenTarget::SPIRVAssembly:
+        return true;
+    }
+}
+
 static bool isD3D11Target(TargetRequest*)
 {
     // We aren't officially supporting D3D11 right now
@@ -886,21 +900,9 @@ static bool isSM5_1OrLater(TargetRequest* targetReq)
 
 static bool isVulkanTarget(TargetRequest* targetReq)
 {
-    switch( targetReq->target )
-    {
-    default:
-        return false;
-
-    case CodeGenTarget::GLSL:
-    case CodeGenTarget::SPIRV:
-    case CodeGenTarget::SPIRVAssembly:
-        break;
-    }
-
-    // For right now, any GLSL-related target is assumed
+    // For right now, any Khronos-related target is assumed
     // to be a Vulkan target.
-
-    return true;
+    return isKhronosTarget(targetReq);
 }
 
 static bool shouldAllocateRegisterSpaceForParameterBlock(