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

Overlapping explicit binding of uniform buffer produces incorrect values #4700

Closed
sivansh11 opened this issue Jul 22, 2024 · 16 comments · Fixed by #4708
Closed

Overlapping explicit binding of uniform buffer produces incorrect values #4700

sivansh11 opened this issue Jul 22, 2024 · 16 comments · Fixed by #4708
Assignees
Labels
goal:client support Feature or fix needed for a current slang user. kind:bug something doesn't work like it should

Comments

@sivansh11
Copy link

explictly setting the same binding and space for multiple uniform buffers doesnt work (gives nonsensical values)

[[vk_binding(0,0)]] uniform readonly uint64_t addresses[1000];
[[vk_binding(0,0)]] uniform readonly uint64_t addresses1[1000];

gives

addresses 0
addresses1 8000

I doubt this is intentional, do let me know if I am wrong.

on commit 5da06d4

@ArielG-NV ArielG-NV added kind:bug something doesn't work like it should goal:client support Feature or fix needed for a current slang user. labels Jul 22, 2024
@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 22, 2024

edit: answered in another comment.

@ArielG-NV ArielG-NV changed the title explict binding for aliasing uniform buffers Overlapping explicit binding of uniform buffers produces incorrect values Jul 22, 2024
@ArielG-NV ArielG-NV changed the title Overlapping explicit binding of uniform buffers produces incorrect values Overlapping explicit binding of uniform buffer produces incorrect values Jul 22, 2024
@ArielG-NV ArielG-NV self-assigned this Jul 22, 2024
@csyonghe
Copy link
Collaborator

Aliasing binding is allowed, but the code isn’t using the right syntax to define buffers. A buffer should be declared as StructuredBuffer, but the vk_binding in the provided code is declared directly on an ordinary data field that will be placed in the default constant buffer, the vk_binding will have no meaning in this case.

you probably want:

[vk::binding(0,0)] ConstantBuffer<uint[1000]> addresses;
[vk::binding(0,0)] ConstantBuffer<uint[1000]> addresses1;

which defines two constant buffer resources that maps to a same binding. Note that global ordinary iniform data cannot have a binding.

@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 22, 2024

Note that global ordinary uniform data cannot have a binding.

@csyonghe should this also produce a warning?

@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 22, 2024

or should we allow automatically mapping [[vk_binding(0,0)]] to equivalent layout syntax in this senario?

(or maybe do both?)

@csyonghe
Copy link
Collaborator

I would say a warning for now on binding attributes declared on uniform data.

@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 22, 2024

I was reading our user guide and it seems we don't explicitly support or don't support the behavior described in this issue, this should be changed as per discussion and issue

Binding information for Vulkan (and OpenGL) may be specified using [[vk::binding(...)]] attributes.

into

Binding information for Vulkan (and OpenGL) may be specified using [[vk::binding(...)]] attributes. It is highly recommended to use ConstantBuffer<T> instead of uniform with vk::binding.

@sivansh11
Copy link
Author

@csyonghe ahh got you, didnt know ordinary uniforms couldnt have an explicit binding,

I tried out using ConstantBuffer, but I am getting a segfault

[[vk::binding(0, 0)]] uniform ConstantBuffer<uint64_t[1000]> addresses;
[[vk::binding(0, 0)]] uniform ConstantBuffer<uint64_t[1000]> addresses1;

[shader("compute")]
[numthreads(1, 1, 1)]
void computeMain(uint3 dispatchThreadID: SV_DispatchThreadID, uint groupIndex: SV_GroupIndex) {
    printf("\nfrom gpu: %llu", addresses1[0]);
    printf("\nfrom gpu: %llu", addresses1[1]);
}

my usage should be correct now ?

I am using u64 to debug because those are VkDeviceAddresses.
In actuallity I want to use pointers, I was hoping that I could just have an array of void *, but that causes another set of problems in slang (issue #4674)

@ArielG-NV
Copy link
Contributor

I tried out using ConstantBuffer, but I am getting a segfault

What would be the compile command used if I may ask?

@csyonghe
Copy link
Collaborator

Looks like it is the array directly inside a ConstantBuffer causing error. This works:

struct V {uint64_t a[1000];};

[[vk::binding(0, 0)]] uniform ConstantBuffer<V> addresses;
[[vk::binding(0, 0)]] uniform ConstantBuffer<V> addresses1;

[shader("compute")]
[numthreads(1, 1, 1)]
void computeMain(uint3 dispatchThreadID: SV_DispatchThreadID, uint groupIndex: SV_GroupIndex) {
    printf("\nfrom gpu: %llu", addresses.a[0]);
    printf("\nfrom gpu: %llu", addresses1.a[1]);
}

@sivansh11
Copy link
Author

@ArielG-NV I am using the c++ api,

DebugInformation SLANG_DEBUG_INFO_LEVEL_MAXIMAL
Optimization SLANG_OPTIMIZATION_LEVEL_NONE
GLSLForceScalarLayout 1

@sivansh11
Copy link
Author

sivansh11 commented Jul 22, 2024

Looks like it is the array directly inside a ConstantBuffer causing error. This works:

struct V {uint64_t a[1000];};

[[vk::binding(0, 0)]] uniform ConstantBuffer<V> addresses;
[[vk::binding(0, 0)]] uniform ConstantBuffer<V> addresses1;

[shader("compute")]
[numthreads(1, 1, 1)]
void computeMain(uint3 dispatchThreadID: SV_DispatchThreadID, uint groupIndex: SV_GroupIndex) {
    printf("\nfrom gpu: %llu", addresses.a[0]);
    printf("\nfrom gpu: %llu", addresses1.a[1]);
}

it compiles now, but I am still getting a validation error while trying to create the shader module

Validation Error: [ UNASSIGNED-Debug-Printf ] | MessageID = 0x9472fbd3 | vkCreateShaderModule():  Error during shader instrumentation in spirv-opt: line 71: ID '50' decorated with Block multiple times is not allowed.
  %V_natural = OpTypeStruct %_Array_natural_uint641000

if you are able to run it, may I know the commit you are on ?

@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 22, 2024

@ArielG-NV I am using the c++ api,

DebugInformation SLANG_DEBUG_INFO_LEVEL_MAXIMAL Optimization SLANG_OPTIMIZATION_LEVEL_NONE GLSLForceScalarLayout 1

I will look into why

[[vk::binding(0, 0)]] uniform ConstantBuffer<uint64_t[1000]> addresses;
[[vk::binding(0, 0)]] uniform ConstantBuffer<uint64_t[1000]> addresses1;

crashes.

(issue #4704)

@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 22, 2024

Update: #4704 has a PR out, uniform ConstantBuffer<uint64_t[1000]> addresses; should work with the PR.

it compiles now, but I am still getting a validation error while trying to create the shader module

I will now checkout this issue.

@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 22, 2024

@sivansh11 I recommend updating your Vulkan SDK to the latest one. I seem to have no trouble compiling the suggested code with Vulkan SDK version 1.3.283.0

struct V {uint64_t a[1000];};

[[vk::binding(0, 0)]] uniform ConstantBuffer<V> addresses;
[[vk::binding(0, 0)]] uniform ConstantBuffer<V> addresses1;

[shader("compute")]
[numthreads(1, 1, 1)]
void computeMain(uint3 dispatchThreadID: SV_DispatchThreadID, uint groupIndex: SV_GroupIndex) {
    printf("\nfrom gpu: %llu", addresses.a[0]);
    printf("\nfrom gpu: %llu", addresses1.a[1]);
}

ArielG-NV added a commit to ArielG-NV/slang that referenced this issue Jul 23, 2024
Fixes: shader-slang#4700

Changes:
* If global uniform has explicit bindings code will warn to use `ConstantBuffer<T>` instead where possible.
ArielG-NV added a commit to ArielG-NV/slang that referenced this issue Jul 23, 2024
Fixes: shader-slang#4700

Changes:
* If a uniform object (which uses uniform locations) has explicit bindings we will warn to use `ConstantBuffer<T>` instead
@sivansh11
Copy link
Author

@sivansh11 I recommend updating your Vulkan SDK to the latest one. I seem to have no trouble compiling the suggested code with Vulkan SDK version 1.3.283.0

struct V {uint64_t a[1000];};

[[vk::binding(0, 0)]] uniform ConstantBuffer<V> addresses;
[[vk::binding(0, 0)]] uniform ConstantBuffer<V> addresses1;

[shader("compute")]
[numthreads(1, 1, 1)]
void computeMain(uint3 dispatchThreadID: SV_DispatchThreadID, uint groupIndex: SV_GroupIndex) {
    printf("\nfrom gpu: %llu", addresses.a[0]);
    printf("\nfrom gpu: %llu", addresses1.a[1]);
}

Yes, I am on 1.3.283.0
again this is the error that I am getting while trying to run the suggested code.

Validation Error: [ UNASSIGNED-Debug-Printf ] | MessageID = 0x9472fbd3 | vkCreateShaderModule():  Error during shader instrumentation in spirv-opt: line 71: ID '50' decorated with Block multiple times is not allowed.
  %V_natural = OpTypeStruct %_Array_natural_uint641000

@ArielG-NV
Copy link
Contributor

ArielG-NV commented Jul 23, 2024

Try updating Slang to 2024.1.30

It looks like the Slang version being used is affected by a spirv-opt change which was solved by Slang in a later revision.

If this does-not fix your issue, please add a comment here so I can look into the problem further.

(note: issue was closed due to solving the original problem)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:client support Feature or fix needed for a current slang user. kind:bug something doesn't work like it should
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants