Fix combined sampler documentation and warning (#6207)

* Fix combined sampler documentation and warning

* Update comment, show detailed '-fvk-t-shift' message in warning instead of generic '-fvk-xxx-shift'

* format code

---------

Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com>

Author: cheneym2

docs/user-guide/a2-01-spirv-target-specific.md

@@ -20,7 +20,14 @@ Combined texture sampler
 Slang supports Combined texture sampler such as `Sampler2D`.
 Slang emits SPIR-V code with `OpTypeSampledImage` instruction.
 
-You can specify two different register numbers for each: one for the texture register and another for the sampler register.
+For SPIR-V targets, explicit bindings may be provided through a single `vk::binding` decoration.
+```
+[[vk::binding(1,2)]]
+Sampler2D explicitBindingSampler;
+```
+
+For other targets (HLSL or others) where combined texture samplers are _not_ supported intrinsicly, they are emulated by Slang using separate objects for Texture and Sampler.
+For explicit binding on such targets, you can specify two different register numbers for each: one for the texture register and another for the sampler register.
 ```
 Sampler2D explicitBindingSampler : register(t4): register(s3);
 ```

source/slang/slang-diagnostic-defs.h

@@ -2019,6 +2019,13 @@ DIAGNOSTIC(
     notValidVaryingParameter,
     "parameter '$0' is not a valid varying parameter.")
 
+DIAGNOSTIC(
+    39029,
+    Warning,
+    registerModifierButNoVkBindingNorShift,
+    "shader parameter '$0' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` "
+    "specified for Vulkan, nor is `-fvk-$1-shift` used.")
+
 //
 
 // 4xxxx - IL code generation.

source/slang/slang-parameter-binding.cpp

@@ -1078,10 +1078,32 @@ static void _maybeDiagnoseMissingVulkanLayoutModifier(
     // oversight on their part.
     if (auto registerModifier = varDecl.getDecl()->findModifier<HLSLRegisterSemantic>())
     {
+        auto varType = getType(context->getASTBuilder(), varDecl.as<VarDeclBase>());
+        if (auto textureType = as<TextureType>(varType))
+        {
+            if (textureType->isCombined())
+            {
+                // Recommend [[vk::binding]] but not '-fvk-xxx-shift` for combined texture samplers
+                getSink(context)->diagnose(
+                    registerModifier,
+                    Diagnostics::registerModifierButNoVulkanLayout,
+                    varDecl.getName());
+                return;
+            }
+        }
+
+        UnownedStringSlice registerClassName;
+        UnownedStringSlice registerIndexDigits;
+        splitNameAndIndex(
+            registerModifier->registerName.getContent(),
+            registerClassName,
+            registerIndexDigits);
+
         getSink(context)->diagnose(
             registerModifier,
-            Diagnostics::registerModifierButNoVulkanLayout,
-            varDecl.getName());
+            Diagnostics::registerModifierButNoVkBindingNorShift,
+            varDecl.getName(),
+            registerClassName);
     }
 }
 
@@ -1256,7 +1278,6 @@ static void addExplicitParameterBindings_GLSL(
         return;
     }
 
-
     const auto hlslInfo = _extractLayoutSemanticInfo(context, hlslRegSemantic);
     if (hlslInfo.kind == LayoutResourceKind::None)
     {
@@ -1270,7 +1291,14 @@ static void addExplicitParameterBindings_GLSL(
     if (auto textureType = as<TextureType>(varType))
     {
         if (textureType->isCombined())
+        {
+            if (!warnedMissingVulkanLayoutModifier)
+            {
+                _maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl.as<VarDeclBase>());
+                warnedMissingVulkanLayoutModifier = true;
+            }
             return;
+        }
     }
 
     // Can we map to a Vulkan kind in principal?

tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl

@@ -0,0 +1,32 @@
+//DIAGNOSTIC_TEST:SIMPLE:-target glsl -profile ps_4_0 -entry main -fvk-t-shift 5 all -fvk-t-shift 7 2  -fvk-s-shift -3 0 -fvk-u-shift 1 2 -no-codegen
+//DIAGNOSTIC_TEST:SIMPLE:-target glsl -profile ps_4_0 -entry main -no-codegen
+
+// This tests that combined texture sampler objects which have D3D style register assignments, but no vk::binding,
+// show an appropriate warning.
+// The warning should appear even if the user used -fvk-xxx-shift options, because those options do not serve to map
+// two register assignments of different types into a single vulkan binding.
+
+struct Data
+{
+    float a;
+    int b;
+};
+
+// Neither vk::binding, nor register, no warning
+Sampler2D 		cs0;
+
+// Only vk::binding, no warning
+[[vk::binding(0,0)]]
+Sampler2D 		cs1;
+
+// Both vk::binding and register, no warning
+[[vk::binding(1,0)]]
+Sampler2D 		cs2 : register(s0): register(t0);
+
+// Only register, should warn without recommending vk-xxx-shift, since that would not help map 2 d3d registers to one vk binding.
+Sampler2D 		cs3 : register(s1): register(t1);
+
+float4 main() : SV_TARGET
+{
+	return float4(1, 1, 1, 0);
+}

tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl.1.expected

@@ -0,0 +1,8 @@
+result code = 0
+standard error = {
+tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl(25): warning 39013: shader parameter 'cs3' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan
+Sampler2D       cs3 : register(s1): register(t1);
+                      ^~~~~~~~
+}
+standard output = {
+}

tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl.expected

@@ -0,0 +1,8 @@
+result code = 0
+standard error = {
+tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl(25): warning 39013: shader parameter 'cs3' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan
+Sampler2D       cs3 : register(s1): register(t1);
+                      ^~~~~~~~
+}
+standard output = {
+}

tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl

@@ -18,4 +18,4 @@ RWStructuredBuffer<int> u2 : register(u3, space2);
 float4 main() : SV_TARGET
 {
 	return float4(1, 1, 1, 0);
-}
+}

tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl.expected

@@ -1,9 +1,9 @@
 result code = -1
 standard error = {
-tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl(11): warning 39013: shader parameter 'c' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan
+tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl(11): warning 39029: shader parameter 'c' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan, nor is `-fvk-b-shift` used.
 ConstantBuffer<Data> c : register(b2);
                          ^~~~~~~~
-tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl(15): warning 39013: shader parameter 'u' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan
+tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl(15): warning 39029: shader parameter 'u' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan, nor is `-fvk-u-shift` used.
 RWStructuredBuffer<Data> u : register(u11);
                              ^~~~~~~~
 tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl(15): error 39025: conflicting vulkan inferred binding for parameter 'c' overlap is 0 and 0

tests/diagnostics/vk-bindings.slang.expected

@@ -1,6 +1,6 @@
 result code = -1
 standard error = {
-tests/diagnostics/vk-bindings.slang(6): warning 39013: shader parameter 't' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan
+tests/diagnostics/vk-bindings.slang(6): warning 39029: shader parameter 't' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan, nor is `-fvk-t-shift` used.
 Texture2D t : register(t0);
               ^~~~~~~~
 tests/diagnostics/vk-bindings.slang(14): error 39015: shader parameter 'b' consumes whole descriptor sets, so the binding must be in the form '[[vk::binding(0, ...)]]'; the non-zero binding '2' is not allowed