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>

9 files changed, 98 insertions, 8 deletions

diff --git a/docs/user-guide/a2-01-spirv-target-specific.md b/docs/user-guide/a2-01-spirv-target-specific.md
index dcbe44e96..5090caacf 100644
--- a/docs/user-guide/a2-01-spirv-target-specific.md
+++ b/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);
 ```
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index cf338edfa..345be7a54 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/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.
diff --git a/source/slang/slang-parameter-binding.cpp b/source/slang/slang-parameter-binding.cpp
index 55c487e4e..61226f898 100644
--- a/source/slang/slang-parameter-binding.cpp
+++ b/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?
diff --git a/tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl b/tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl
new file mode 100644
index 000000000..1506c638d
--- /dev/null
+++ b/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);
+}
diff --git a/tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl.1.expected b/tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl.1.expected
new file mode 100644
index 000000000..87198b248
--- /dev/null
+++ b/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 = {
+}
diff --git a/tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl.expected b/tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl.expected
new file mode 100644
index 000000000..87198b248
--- /dev/null
+++ b/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 = {
+}
diff --git a/tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl b/tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl
index 4c98b4e93..9734fda56 100644
--- a/tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl
+++ b/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);
-}
\ No newline at end of file
+}
diff --git a/tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl.expected b/tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl.expected
index 907572f9e..1f1e1a5a6 100644
--- a/tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl.expected
+++ b/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
diff --git a/tests/diagnostics/vk-bindings.slang.expected b/tests/diagnostics/vk-bindings.slang.expected
index 6997a7809..901705566 100644
--- a/tests/diagnostics/vk-bindings.slang.expected
+++ b/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