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.

1 files changed, 103 insertions, 28 deletions

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