From 2ba6458eba8bd2d4f4d2ffdd452ae089e5b50907 Mon Sep 17 00:00:00 2001 From: cheneym2 Date: Wed, 29 Jan 2025 14:59:42 -0500 Subject: 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> --- docs/user-guide/a2-01-spirv-target-specific.md | 9 +++++- source/slang/slang-diagnostic-defs.h | 7 +++++ source/slang/slang-parameter-binding.cpp | 34 ++++++++++++++++++++-- .../hlsl-to-vulkan-sampler-diagnostic.hlsl | 32 ++++++++++++++++++++ ...sl-to-vulkan-sampler-diagnostic.hlsl.1.expected | 8 +++++ ...hlsl-to-vulkan-sampler-diagnostic.hlsl.expected | 8 +++++ .../hlsl-to-vulkan-shift-diagnostic.hlsl | 2 +- .../hlsl-to-vulkan-shift-diagnostic.hlsl.expected | 4 +-- tests/diagnostics/vk-bindings.slang.expected | 2 +- 9 files changed, 98 insertions(+), 8 deletions(-) create mode 100644 tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl create mode 100644 tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl.1.expected create mode 100644 tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl.expected 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()) { + auto varType = getType(context->getASTBuilder(), varDecl.as()); + if (auto textureType = as(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(varType)) { if (textureType->isCombined()) + { + if (!warnedMissingVulkanLayoutModifier) + { + _maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl.as()); + 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 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 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 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 -- cgit v1.2.3