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

[[vk::binding]] ignored(?) in parameter lists #5626

Open
pdjonov opened this issue Nov 21, 2024 · 5 comments
Open

[[vk::binding]] ignored(?) in parameter lists #5626

pdjonov opened this issue Nov 21, 2024 · 5 comments
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang kind:bug something doesn't work like it should

Comments

@pdjonov
Copy link

pdjonov commented Nov 21, 2024

This works:

[shader("vertex")]
FragIn_1uv vs_1uv(
    [[vk::push_constant]] uniform Transform transform,
    in VertIn_1uv in)

And I would therefore expect this to also work:

struct FragIn
{
    float4 Position : SV_Position;

    float3 ViewNorm;
    float2 TexCoord;
};

[shader("fragment")]
float4 frag(
    in FragIn in,
    [[vk::binding(0, 1)]] uniform Sampler2D Diffuse) : SV_Target

And it does compile, but the [[vk::binding]] gets ignored and Diffuse winds up in an automatically assigned descriptor in set zero.

The following works as I'd expect:

[[vk::binding(0, 1)]]
uniform Sampler2D Diffuse;

[shader("fragment")]
float4 frag(
    in FragIn in) : SV_Target

Using the Slang binaries in Vulkan SDK 1.3.296.0 (slang::IGlobalSession::getBuildTagString says "2024.13").

Thank you.

@bmillsNV bmillsNV added kind:bug something doesn't work like it should goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang labels Nov 21, 2024
@bmillsNV bmillsNV added this to the Q4 2024 (Fall) milestone Nov 21, 2024
@csyonghe
Copy link
Collaborator

csyonghe commented Nov 22, 2024

Currently explicit binding is only supported for global scope declarations, but support for entrypoint parameters should be easy to add.

@pdjonov
Copy link
Author

pdjonov commented Nov 22, 2024

Maybe I don't understand the design philosophy behind the difference (and maybe I skimmed past somewhere in the docs that explains it), but to me as a user these look like just two different scopes for all the same things (except, obviously, type declarations would be a weird thing to try and put in a parameter block).

At the very least I think the [[vk::*]] directives should:

  • be consistent in where they're supported (and making them supported in both scopes would satisfy this),
  • and generate an error (or at the very least warnings) where they're not.

Like if someone's taking the time to type them in then they're probably trying to match a layout somewhere else in their code that isn't coming from "use reflection to inspect whatever slang happens to have done". Compilation shouldn't just report "success" if explicit layout directives can't be honored.

Tangentially related question: is there a clear example of how I can take the linked together module from which I'm getting my SPIR-V and generate a VK pipeline layout from it? The reflection API isn't the easiest thing get up to speed with, and reflecting over the generated SPIR-V is kinda noisy given all the internally generated types and how everything gets nested into one.

@csyonghe
Copy link
Collaborator

csyonghe commented Nov 22, 2024

I agree with you that our reflection API isn't ideal. We are working to address this, and likely going to create a sample code to do exactly that: go from slang reflection to vulkan pipeline layout in explained code.

I consider the missing support of [vk::binding] on entrypoints a bug and we will fix this.

In the meanwhile, you can take a look at the reflection json to get a better understanding of what the reflection api is providing. You can explore this on the playground too.

@pdjonov
Copy link
Author

pdjonov commented Nov 23, 2024

Another nice to have: it'd be super cool if I could globally (say, for a compilation session):

  • make it an error to have anything go into push constants without an explicit [[vk::push_constants]] annotation,
  • and tell the compiler "you own descriptor set n, bind nothing to any other set without an explicit [[vk:binding]] telling you to do so.

Basically, I like the idea of letting slang automate descriptor layouts for most of my pipeline interface, but I need it to stay out of push constants and the fixed descriptors that the engine provides, and that means it has to stay out of sets [0..n) unless told otherwise. Otherwise I have to explicitly bind all the variables.

@obhi-d
Copy link

obhi-d commented Nov 27, 2024

I would also be interested in this fix, as this should be an intermediate fix for #5595, thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang kind:bug something doesn't work like it should
Projects
None yet
Development

No branches or pull requests

4 participants