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 1 commit
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-xxx-shift` used.")

//

// 4xxxx - IL code generation.
Expand Down
24 changes: 22 additions & 2 deletions source/slang/slang-parameter-binding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1078,9 +1078,23 @@ 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;
}
}

getSink(context)->diagnose(
registerModifier,
Diagnostics::registerModifierButNoVulkanLayout,
Diagnostics::registerModifierButNoVkBindingNorShift,
varDecl.getName());
}
}
Expand Down Expand Up @@ -1256,7 +1270,6 @@ static void addExplicitParameterBindings_GLSL(
return;
}


const auto hlslInfo = _extractLayoutSemanticInfo(context, hlslRegSemantic);
if (hlslInfo.kind == LayoutResourceKind::None)
{
Expand All @@ -1270,7 +1283,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
30 changes: 30 additions & 0 deletions tests/diagnostics/hlsl-to-vulkan-sampler-diagnostic.hlsl
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
//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, even in the presence of -fvk-xxx-shift options. Warning should not recommend using -fvk-xxx-shift.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment here may need to be updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, though I'm not sure if my update meets your expectations. What I said in this version of the comment is what I believe to be correct. Hoping it's just a matter of unclear wording.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, never mind, the comment is correct. I misunderstood it.


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-xxx-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-xxx-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-xxx-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