Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix combined sampler documentation and warning #6207

Merged
merged 5 commits into from
Jan 29, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion docs/user-guide/a2-01-spirv-target-specific.md
Original file line number Diff line number Diff line change
Expand Up @@ -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);
```
Expand Down
7 changes: 7 additions & 0 deletions source/slang/slang-diagnostic-defs.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
34 changes: 31 additions & 3 deletions source/slang/slang-parameter-binding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -1256,7 +1278,6 @@ static void addExplicitParameterBindings_GLSL(
return;
}


const auto hlslInfo = _extractLayoutSemanticInfo(context, hlslRegSemantic);
if (hlslInfo.kind == LayoutResourceKind::None)
{
Expand All @@ -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?
Expand Down
32 changes: 32 additions & 0 deletions tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl
Original file line number Diff line number Diff line change
@@ -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);
}
Original file line number Diff line number Diff line change
@@ -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 = {
}
Original file line number Diff line number Diff line change
@@ -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 = {
}
2 changes: 1 addition & 1 deletion tests/diagnostics/hlsl-to-vulkan-shift-diagnostic.hlsl
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@ RWStructuredBuffer<int> u2 : register(u3, space2);
float4 main() : SV_TARGET
{
return float4(1, 1, 1, 0);
}
}
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/diagnostics/vk-bindings.slang.expected
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Loading