Improvements around HLSLToVulkanLayout (#2867)

* #include an absolute path didn't work - because paths were taken to always be relative.

* Improve the HLSLToVulkanLayoutOptions interface.
Add more diagnostics.
Add diagnostics test.

* Add check for global binding using file check.

* Fix issues with some tests around making some diagnostics ids unique.

* Small improvements with doc/handling of vk-<>-shift option setup.

---------

Co-authored-by: Yong He <yonghe@outlook.com>

5 files changed, 152 insertions, 83 deletions

diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index e8d521630..a11474c8f 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -551,19 +551,24 @@ DIAGNOSTIC(39013, Warning, registerModifierButNoVulkanLayout, "shader parameter
 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")
+DIAGNOSTIC(39016, Warning, hlslToVulkanMappingNotFound, "unable to infer Vulkan binding for '$0', automatic layout will be used")
 
-DIAGNOSTIC(39016, Warning, globalUniformNotExpected, "'$0' is implicitly a global shader parameter, not a global variable. If a global variable is intended, add the 'static' modifier. If a uniform shader parameter is intended, add the 'uniform' modifier to silence this warning.")
+DIAGNOSTIC(39017, Error, dontExpectOutParametersForStage, "the '$0' stage does not support `out` or `inout` entry point parameters")
+DIAGNOSTIC(39018, Error, dontExpectInParametersForStage, "the '$0' stage does not support `in` entry point parameters")
 
-DIAGNOSTIC(39017, Error, tooManyShaderRecordConstantBuffers, "can have at most one 'shader record' attributed constant buffer; found $0.")
+DIAGNOSTIC(39019, Warning, globalUniformNotExpected, "'$0' is implicitly a global shader parameter, not a global variable. If a global variable is intended, add the 'static' modifier. If a uniform shader parameter is intended, add the 'uniform' modifier to silence this warning.")
 
-DIAGNOSTIC(39018, Error, typeParametersNotAllowedOnEntryPointGlobal, "local-root-signature shader parameter '$0' at global scope must not include existential/interface types")
+DIAGNOSTIC(39020, Error, tooManyShaderRecordConstantBuffers, "can have at most one 'shader record' attributed constant buffer; found $0.")
 
-DIAGNOSTIC(39019, Warning, vkIndexWithoutVkLocation, "ignoring '[[vk::index(...)]]` attribute without a corresponding '[[vk::location(...)]]' attribute")
-DIAGNOSTIC(39020, Error, mixingImplicitAndExplicitBindingForVaryingParams, "mixing explicit and implicit bindings for varying parameters is not supported (see '$0' and '$1')")
+DIAGNOSTIC(39021, Error, typeParametersNotAllowedOnEntryPointGlobal, "local-root-signature shader parameter '$0' at global scope must not include existential/interface types")
+
+DIAGNOSTIC(39022, Warning, vkIndexWithoutVkLocation, "ignoring '[[vk::index(...)]]` attribute without a corresponding '[[vk::location(...)]]' attribute")
+DIAGNOSTIC(39023, Error, mixingImplicitAndExplicitBindingForVaryingParams, "mixing explicit and implicit bindings for varying parameters is not supported (see '$0' and '$1')")
+
+DIAGNOSTIC(39024, Warning, cannotInferVulkanBindingWithoutRegisterModifier, "shader parameter '$0' doesn't have a 'register' specified, automatic layout will be used")
 
 //
+
 // 4xxxx - IL code generation.
 //
 DIAGNOSTIC(40001, Error, bindingAlreadyOccupiedByComponent, "resource binding location '$0' is already occupied by component '$1'.")
diff --git a/source/slang/slang-hlsl-to-vulkan-layout-options.cpp b/source/slang/slang-hlsl-to-vulkan-layout-options.cpp
index e143762a1..bb02a079c 100644
--- a/source/slang/slang-hlsl-to-vulkan-layout-options.cpp
+++ b/source/slang/slang-hlsl-to-vulkan-layout-options.cpp
@@ -27,14 +27,25 @@ static NamesDescriptionValue s_vulkanShiftKinds[] =
 
 HLSLToVulkanLayoutOptions::HLSLToVulkanLayoutOptions()
 {
+    reset();
+    SLANG_ASSERT(isReset());
+}
+
+void HLSLToVulkanLayoutOptions::setGlobalsBinding(const Binding& binding)
+{
+    m_globalsBinding = binding;
+}
+
+void HLSLToVulkanLayoutOptions::reset()
+{
     for (auto& shift : m_allShifts)
     {
         shift = kInvalidShift;
     }
-}
 
+    m_shifts.clear();
+}
 
-   /// Set the the all option for the kind
 void HLSLToVulkanLayoutOptions::setAllShift(Kind kind, Index shift)
 {
     // We try to follow the convention, of the *last* entry set is the one used.
@@ -76,29 +87,38 @@ Index HLSLToVulkanLayoutOptions::getShift(Kind kind, Index set) const
     return m_allShifts[Index(kind)];
 }
 
-bool HLSLToVulkanLayoutOptions::isDefault() const
+bool HLSLToVulkanLayoutOptions::canInferBindings() const
 {
     // If any all shift is set it's not default
     for (auto shift : m_allShifts)
     {
         if (shift != kInvalidShift)
         {
-            return false;
+            return true;
         }
     }
 
-    // If any has a non zero shift, it's not default
-    for (auto& pair : m_shifts)
+    return m_shifts.getCount() > 0;
+}
+
+bool HLSLToVulkanLayoutOptions::hasState() const
+{
+    return canInferBindings() || hasGlobalsBinding();
+}
+
+HLSLToVulkanLayoutOptions::Binding HLSLToVulkanLayoutOptions::inferBinding(Kind kind, const Binding& inBinding) const
+{
+    auto shift = getShift(kind, inBinding.set);
+
+    if (shift != kInvalidShift)
     {
-        // We need a value that is non zero...
-        if (pair.value)
-        {
-            return false;
-        }
+        Binding binding(inBinding);
+        binding.index += shift;
+        return binding;
     }
 
-    // If either has been set it's not default
-    return m_globalsBinding < 0 && m_globalsBindingSet < 0;
+    // Else return an invalid binding
+    return Binding();
 }
 
 /* static */HLSLToVulkanLayoutOptions::Kind HLSLToVulkanLayoutOptions::getKind(slang::ParameterCategory param)
diff --git a/source/slang/slang-hlsl-to-vulkan-layout-options.h b/source/slang/slang-hlsl-to-vulkan-layout-options.h
index 9260fe91d..642f6b5f4 100644
--- a/source/slang/slang-hlsl-to-vulkan-layout-options.h
+++ b/source/slang/slang-hlsl-to-vulkan-layout-options.h
@@ -21,6 +21,17 @@ public:
 
     static const Index kInvalidShift = Index(0x80000000);
 
+        /// For holding combination of set and index for binding
+    struct Binding
+    {
+        bool isSet() const { return set >= 0 && index >= 0;}
+        void reset() { set = -1; index = -1; }
+        bool isInvalid() const { return !isSet(); }
+
+        Index set = -1;
+        Index index = -1;
+    };
+
     // {b|s|t|u} 
     enum class Kind
     {
@@ -56,23 +67,44 @@ public:
         /// Get the shift. Returns kInvalidShift if no shift is found
     Index getShift(Kind kind, Index set) const;
 
-        /// Returns true if contains default information. If so it can in effect be ignored
-    bool isDefault() const;
-
         /// True as global binds set
-    bool hasGlobalsBinding() const { return m_globalsBinding >= 0 && m_globalsBindingSet >= 0; }
+    bool hasGlobalsBinding() const { return m_globalsBinding.isSet(); }
+
+        /// True if holds state such that vulkan bindings can be inferred from HLSL bindings
+    bool canInferBindings() const;
+
+        /// Given an kind and a binding infer the vulkan binding.
+        /// Will return an invalid binding if one is not found
+    Binding inferBinding(Kind kind, const Binding& inBinding) const;
+
+        /// Reset state such that all options are set to their default. The same state as when 
+        /// originally constructed
+    void reset();
+
+        /// Returns true if any state is set
+    bool hasState() const;
+
+        /// Returns true if contains default reset state. If so it can in effect be ignored
+    bool isReset() const { return !hasState(); }
+
+        /// Set the global binding
+    void setGlobalsBinding(Index set, Index bindingIndex) { setGlobalsBinding(Binding{set, bindingIndex}); }
+        /// Set the global bindings
+    void setGlobalsBinding(const Binding& binding);
+        /// Get the globals binding
+    const Binding& getGlobalsBinding() const { return m_globalsBinding; }
 
         /// Ctor
     HLSLToVulkanLayoutOptions();
-
+    
         /// Get information about the different kinds
     static ConstArrayView<NamesDescriptionValue> getKindInfos();
 
         /// Given a paramCategory get the kind. Returns Kind::Invalid if not an applicable category
     static Kind getKind(slang::ParameterCategory param);
 
-    Index m_globalsBinding = -1;
-    Index m_globalsBindingSet = -1;
+protected:
+    Binding m_globalsBinding;
 
     Index m_allShifts[Count(Kind::CountOf)];
 
diff --git a/source/slang/slang-options.cpp b/source/slang/slang-options.cpp
index 0c788a404..cb04632cc 100644
--- a/source/slang/slang-options.cpp
+++ b/source/slang/slang-options.cpp
@@ -422,6 +422,17 @@ void initCommandOptions(CommandOptions& options)
 
     options.setCategory("Target");
 
+    StringBuilder vkShiftNames;
+    {
+        for (auto nameSlice : NameValueUtil::getNames(NameValueUtil::NameKind::All, HLSLToVulkanLayoutOptions::getKindInfos()))
+        {
+            // -fvk-{b|s|t|u}-shift
+            vkShiftNames << "-fvk-" << nameSlice << "-shift,";
+        }
+        // remove last ,
+        vkShiftNames.reduceLength(vkShiftNames.getLength() - 1);
+    }
+
     const Option targetOpts[] = 
     {
         { OptionKind::Capability, "-capability", "-capability <capability>[+<capability>...]",
@@ -447,9 +458,15 @@ void initCommandOptions(CommandOptions& options)
         { OptionKind::GLSLForceScalarLayout,
          "-force-glsl-scalar-layout", nullptr,
          "Force using scalar block layout for uniform and shader storage buffers in GLSL output."},
+        { OptionKind::VulkanBindShift, vkShiftNames.getBuffer(), "-vk-<vulkan-shift>-shift <N> <space>", 
+        "For example '-vk-b-shift <N> <space>' shifts by N the inferred binding numbers for all resources in 'b' registers of space <space>. "
+        "For a resource attached with :register(bX, <space>) but not [vk::binding(...)], "
+        "sets its Vulkan descriptor set to <space> and binding number to X + N. If you need to shift the "
+        "inferred binding numbers for more than one space, provide more than one such option. "
+        "If more than one such option is provided for the same space, the last one takes effect. "
+        "If you need to shift the inferred binding numbers for all sets, use 'all' as <space>." },
         { OptionKind::VulkanBindGlobals, "-fvk-bind-globals", "-fvk-bind-globals <N> <descriptor-set>",
-        "Places the $Globals cbuffer at descriptor set <descriptor-set> and binding <N>. See HLSL global variables and Vulkan binding for explanation and examples."
-        },
+        "Places the $Globals cbuffer at descriptor set <descriptor-set> and binding <N>."},
         { OptionKind::EnableEffectAnnotations,
          "-enable-effect-annotations", nullptr, 
          "Enables support for legacy effect annotation syntax."},
@@ -457,25 +474,6 @@ void initCommandOptions(CommandOptions& options)
 
     _addOptions(makeConstArrayView(targetOpts), options);
 
-    {
-        StringBuilder names;
-        for (auto nameSlice : NameValueUtil::getNames(NameValueUtil::NameKind::All, HLSLToVulkanLayoutOptions::getKindInfos()))
-        {
-            // -fvk-{b|s|t|u}-shift
-            names << "-fvk-" << nameSlice << "-shift,";
-        }
-        // remove last ,
-        names.reduceLength(names.getLength() - 1);
-        options.add(names.getBuffer(), "-vk-<vulkan-shift>-shift <N> <space>", 
-            "Shifts by N the inferred binding numbers for all resources in b-type registers of space <space>. "
-            "Specifically, for a resouce attached with :register(bX, <space>) but not [vk::binding(...)], "
-            "sets its Vulkan descriptor set to <space> and binding number to X + N. If you need to shift the "
-            "inferred binding numbers for more than one space, provide more than one such option. "
-            "If more than one such option is provided for the same space, the last one takes effect. "
-            "If you need to shift the inferred binding numbers for all sets, use 'all' as <space>.", 
-            UserValue(OptionKind::VulkanBindShift));
-    }
-
     /* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Downstream !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! */
 
     options.setCategory("Downstream");
@@ -1940,7 +1938,7 @@ SlangResult OptionsParser::_parse(
             }
             case OptionKind::VulkanBindShift:
             {
-                // -fvk-{b|s|t|u}-shift
+                // -fvk-{b|s|t|u}-shift <binding-shift> <set>
                 const auto slice = arg.value.getUnownedSlice().subString(5, 1);
                 HLSLToVulkanLayoutOptions::Kind kind;
                 SLANG_RETURN_ON_FAIL(_getValue(arg, slice, kind));
@@ -1963,13 +1961,12 @@ SlangResult OptionsParser::_parse(
             }
             case OptionKind::VulkanBindGlobals:
             {
-                // -fvk-bind-globals N M
+                // -fvk-bind-globals <index> <set>
                 Int binding, bindingSet;
                 SLANG_RETURN_ON_FAIL(_expectInt(arg, binding));
                 SLANG_RETURN_ON_FAIL(_expectInt(arg, bindingSet));
 
-                m_hlslToVulkanLayoutOptions->m_globalsBindingSet = Index(bindingSet);
-                m_hlslToVulkanLayoutOptions->m_globalsBinding = Index(binding);
+                m_hlslToVulkanLayoutOptions->setGlobalsBinding(Index(bindingSet), Index(binding));
                 break;
             }
             case OptionKind::Profile: SLANG_RETURN_ON_FAIL(_parseProfile(arg)); break;
@@ -2295,7 +2292,7 @@ SlangResult OptionsParser::_parse(
     }
 
     // If there are no layout settings, we don't need to carry this state
-    if (m_hlslToVulkanLayoutOptions->isDefault())
+    if (m_hlslToVulkanLayoutOptions->isReset())
     {
         m_hlslToVulkanLayoutOptions.setNull();
     }
diff --git a/source/slang/slang-parameter-binding.cpp b/source/slang/slang-parameter-binding.cpp
index fbd11c7d0..f6637b13c 100644
--- a/source/slang/slang-parameter-binding.cpp
+++ b/source/slang/slang-parameter-binding.cpp
@@ -1052,53 +1052,66 @@ static void addExplicitParameterBindings_GLSL(
         return;
     }
 
-    // See if we can infer vulkan binding from HLSL if we have such options set
+    // See if we can infer vulkan binding from HLSL if we have such options set, we know 
+    // we can't map
     auto hlslToVulkanLayoutOptions = context->getTargetRequest()->getHLSLToVulkanLayoutOptions();
 
-    if (!hlslToVulkanLayoutOptions)
+    // If we have the options, but cannot infer bindings, we don't need to go further
+    if (hlslToVulkanLayoutOptions == nullptr || !hlslToVulkanLayoutOptions->canInferBindings())
     {
         _maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl);
         return;
     }
 
-    // Do we have any vulkan shift settings
+    // We need an HLSL register semantic to to infer from
     auto hlslRegSemantic = varDecl.getDecl()->findModifier<HLSLRegisterSemantic>();
-
     if (hlslRegSemantic == nullptr)
     {
-        // We don't have a HLSL binding, so we can't infer, so we can't assign an infered explicit binding
+        // We don't have a HLSL binding, so no inferance can occur, issue a warning
+        // 
+        // TODO(JS): I suppose there is some ambiguity here, because if we did a semantic lookup, and it didn't have a vulkanKind
+        // or didn't parse correctly we wouldn't issue this message.
+        getSink(context)->diagnose(varDecl, Diagnostics::cannotInferVulkanBindingWithoutRegisterModifier, varDecl);
         return;
     }
             
     // Get the HLSL binding info
     const auto hlslInfo = ExtractLayoutSemanticInfo(context, hlslRegSemantic);
-    if (hlslInfo.kind != LayoutResourceKind::None)
+    if (hlslInfo.kind == LayoutResourceKind::None)
     {
-        // We need to map to the GLSL binding types
-        HLSLToVulkanLayoutOptions::Kind vulkanKind = HLSLToVulkanLayoutOptions::getKind(hlslInfo.kind);
-        if (vulkanKind != HLSLToVulkanLayoutOptions::Kind::Invalid)
-        {
-            const auto shift = hlslToVulkanLayoutOptions->getShift(vulkanKind, Index(hlslInfo.space));
-            if (shift != HLSLToVulkanLayoutOptions::kInvalidShift)
-            {
-                const Index bindingIndex = Index(hlslInfo.index) + shift;
+        // Is no hlsl resource binding
+        return;
+    }
 
-                if (bindingIndex >= 0)
-                {
-                    // Add for descriptor slot 
-                    resInfo = typeLayout->findOrAddResourceInfo(LayoutResourceKind::DescriptorTableSlot);
+    // We need to map to the GLSL binding types
+    HLSLToVulkanLayoutOptions::Kind vulkanKind = HLSLToVulkanLayoutOptions::getKind(hlslInfo.kind);
+    if (vulkanKind == HLSLToVulkanLayoutOptions::Kind::Invalid)
+    {
+        // The binding is not "inferable" so we are done
+        return;
+    }
 
-                    semanticInfo.kind = resInfo->kind;
-                    semanticInfo.index = UInt(bindingIndex);
-                    semanticInfo.space = hlslInfo.space;
+    const auto hlslBinding = HLSLToVulkanLayoutOptions::Binding{ Index(hlslInfo.space), Index(hlslInfo.index) };
+    const auto vulkanBinding = hlslToVulkanLayoutOptions->inferBinding(vulkanKind, hlslBinding);
+    
+    if (vulkanBinding.isInvalid())
+    {
+        // If we made it here, there are shift options, but there isn't one for the space/kind specified
+        // That could be a problem and unexpected, so issue a warning
+        getSink(context)->diagnose(varDecl, Diagnostics::hlslToVulkanMappingNotFound, varDecl);
+        return;
+    }
+
+    // Add for descriptor slot 
+    resInfo = typeLayout->findOrAddResourceInfo(LayoutResourceKind::DescriptorTableSlot);
+
+    semanticInfo.kind = resInfo->kind;
+    semanticInfo.index = UInt(vulkanBinding.index);
+    semanticInfo.space = UInt(vulkanBinding.set);
                     
-                    const LayoutSize count = resInfo->count;
+    const LayoutSize count = resInfo->count;
 
-                    addExplicitParameterBinding(context, parameterInfo, varDecl, semanticInfo, count);
-                }
-            }
-        }
-    }
+    addExplicitParameterBinding(context, parameterInfo, varDecl, semanticInfo, count);
 }
 
 // Given a single parameter, collect whatever information we have on
@@ -3709,16 +3722,18 @@ RefPtr<ProgramLayout> generateParameterBindings(
     // we can't use *that* as the default space, so we allocate if
     if (auto vulkanOptions = targetReq->getHLSLToVulkanLayoutOptions())
     {
-        if (vulkanOptions->hasGlobalsBinding())
+        const auto& globalBinding = vulkanOptions->getGlobalsBinding();
+
+        if (globalBinding.isSet())
         {
             // Create VarLayout which will be associated with the binding, and setup later
             globalScopeVarLayout = new VarLayout;
 
             // Allocate the set
-            markSpaceUsed(&context, nullptr, vulkanOptions->m_globalsBindingSet);
+            markSpaceUsed(&context, nullptr, globalBinding.set);
 
             // Mark the use of this binding
-            globalConstantBufferBinding = _assignConstantBufferBinding(&context, globalScopeVarLayout, vulkanOptions->m_globalsBindingSet, vulkanOptions->m_globalsBinding);
+            globalConstantBufferBinding = _assignConstantBufferBinding(&context, globalScopeVarLayout, globalBinding.set, globalBinding.index);
         }
     }