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

Implicit register binding for hlsl to non-hlsl targets #4338

23 changes: 17 additions & 6 deletions source/slang/slang-parameter-binding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1146,12 +1146,12 @@ static void addExplicitParameterBindings_GLSL(
// See if we can infer vulkan binding from HLSL if we have such options set, we know
// we can't map
auto hlslToVulkanLayoutOptions = context->getTargetProgram()->getHLSLToVulkanLayoutOptions();

bool warnedMissingVulkanLayoutModifier = false;
// If we have the options, but cannot infer bindings, we don't need to go further
if (hlslToVulkanLayoutOptions == nullptr || !hlslToVulkanLayoutOptions->canInferBindings())
{
warnedMissingVulkanLayoutModifier = true;
_maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl.as<VarDeclBase>());
return;
}

// We need an HLSL register semantic to to infer from
Expand Down Expand Up @@ -1190,20 +1190,31 @@ static void addExplicitParameterBindings_GLSL(
// If inference is not enabled for this kind, we can issue a warning
if (!hlslToVulkanLayoutOptions->canInfer(vulkanKind, hlslInfo.space))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Due to your change at line 1154, hlslToVulkanLayoutOptions can be nullptr here.

{
_maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl.as<VarDeclBase>());
return;
if(!warnedMissingVulkanLayoutModifier)
_maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl.as<VarDeclBase>());
}

// We use the HLSL binding directly (even though this notionally for GLSL/Vulkan)
// We'll do the shifting at later later point in _maybeApplyHLSLToVulkanShifts
//resInfo = typeLayout->findOrAddResourceInfo(hlslInfo.kind);
resInfo = typeLayout->findOrAddResourceInfo(hlslInfo.kind);

semanticInfo.kind = resInfo->kind;
semanticInfo.index = UInt(hlslInfo.index);
semanticInfo.space = UInt(hlslInfo.space);


if (resInfo->count.isFinite() && resInfo->count.getFiniteValue() == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what the following code is doing. Why do we set DescriptorTableSlot count to 0?

Copy link
Contributor Author

@ArielG-NV ArielG-NV Jun 11, 2024

Choose a reason for hiding this comment

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

I don't know if this is currently the best way todo this, I will need to look around more

From what I understand, DescriptorTableSlot as a representation does not make sense if we are just mapping 1:1 register(binding,set)->layout(binding=,set=).

setting DescriptorTableSlot to 0 means we won't use its value to offset any future bindings when emitting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

DescriptorTableSlot is binding. We rely on it to report the binding value for vulkan targets, so the DescirptorTableSlot offset should be exactly the same as the explicit binding value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any reason we use DescriptorTableSlot as a binding offset?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't have any other dedicated enum to represent the binding offset inside a descriptor set. We use DescriptorTableSlot to mean exactly that for Vulkan.

Copy link
Contributor Author

@ArielG-NV ArielG-NV Jun 11, 2024

Choose a reason for hiding this comment

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

👍we store data same to how a user would use descriptor sets, that makes sense. I will change the solution accordingly

{
// hlsl objects are not going to set on a descriptor table slot
if (auto descriptorTableSlot = typeLayout->findOrAddResourceInfo(LayoutResourceKind::DescriptorTableSlot))
{
descriptorTableSlot->count = 0;
}
resInfo->count = 1;
}
const LayoutSize count = resInfo->count;


addExplicitParameterBinding(context, parameterInfo, as<VarDeclBase>(varDecl.getDecl()), semanticInfo, count);
}

Expand Down
48 changes: 48 additions & 0 deletions tests/hlsl/register-syntax.slang
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
//TEST:SIMPLE(filecheck=CHECK_GLSL): -target glsl -stage compute -entry computeMain
//TEST:SIMPLE(filecheck=CHECK_HLSL): -target hlsl -stage compute -entry computeMain
//CHECK_HLSL: u4, space2
//CHECK_HLSL: u5, space1
//CHECK_HLSL: u6
//CHECK_HLSL: u7
//CHECK_HLSL: u9
//CHECK_HLSL: u12

//CHECK_GLSL: binding = 4, set = 2
//CHECK_GLSL: binding = 5, set = 1
//CHECK_GLSL: binding = 6
//CHECK_GLSL: binding = 7
//CHECK_GLSL: binding = 10
//CHECK_GLSL: binding = 12
jkwak-work marked this conversation as resolved.
Show resolved Hide resolved

[[vk::binding(4,2)]]
RWStructuredBuffer<int> b0 : register(u4, space2);

RWStructuredBuffer<int> b1 : register(u5, space1);

RWStructuredBuffer<int> b2 : register(u6);

[[vk::binding(7,0)]]
RWStructuredBuffer<int> b3 : register(u7, space0);

[[vk::binding(10,0)]]
RWStructuredBuffer<int> b4[2] : register(u9);

RWStructuredBuffer<int> b5[2] : register(u12);

[numthreads(1, 1, 1)]
void computeMain(int3 dispatchThreadID : SV_DispatchThreadID)
{
int tid = dispatchThreadID.x;
b0[0] = 1;

b1[0] = 1;

b2[0] = 1;

b3[0] = 1;

b4[0][0] = 1;
b4[0][1] = 1;

b5[0][1] = 1;
}
Loading