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

[Vulkan] Incorrect descriptor set when compiling from single slang shader? #669

Closed
vasumahesh1 opened this issue Oct 10, 2018 · 7 comments
Closed
Labels
kind:question questions or discussions

Comments

@vasumahesh1
Copy link

Hi,

It seems I have another descriptor set issue. And a doubt too.

Say if I have:

ParameterBlock<UBOData> ubo;
SamplerState     textureSampler;
Texture2D        textureSource;

And I have both my pixel shader and vertex shader functions in the same file. But ubo is not used in PS and vice versa for the texture & sampler.

Do I expect ubo to be assigned to set 0 and texture sampler to set 1 when I get my compiled output? Or Do I expect both of them to be set 0 (Because the unused variables get compiled out)?

Because I have neither the case and ubo gets assigned to set 1 and the texture+sampler gets set 0 (and correct bindings). Here is the source I used

Output for GLSL:

// Vertex Shader
#line 25
layout(binding = 0, set = 1)
layout(std140) uniform _S1
{
    UBOData_0 ubo_0;
};

// Pixel Shader
#line 27 0
layout(binding = 1)
uniform texture2D textureSource_0;


#line 26
layout(binding = 0)
uniform sampler textureSampler_0;

Tested on 0.11.1

@tangent-vector
Copy link
Contributor

The Short Answer

This behavior is Slang working as intended.

The Slightly Longer Answer

Anything you declare outside of an explicit ParameterBlock needs to go into a "default" descriptor set. The rule that Slang uses right now is that any time it needs a "default" descriptor set it always comes before all the non-default (explicit) ones.

The Even Longer Answer

The right way to think about how Slang assigns sets and bindings is usually to think about the compiler "opening" and then closing certain scopes as it reads your code, where a "scope" could be a descriptor set, or a constant buffer, etc.

Take an example where I put resources inside a constant buffer:

cbuffer C
{
    Texture2D t;
    float4 a;
}

If we imagine how the compiler scans this code to assign bindings, then when it sees the cbuffer declaration it "opens" a new constant buffer, which will get binding=0, and then the first thing it sees inside the storage of that constant buffer is a Texture2D, which will get binding=1.

The compiler does not put off allocating the binding for the cbuffer until it sees the first field that might need it (a in this case). However, if the compiler gets to the end of the cbuffer and "closes" the constant buffer it was building, it will check if the buffer would be empty (because it only contained resource types), and then drop the binding for it. So in a case like the following, the texture t would get binding=0:

cbuffer C
{
    Texture2D t;
}

Now, the same basic logic applies to ParameterBlock layout, but instead of working at the level of bindings, a parameter block works at the level of sets. So if I have the following:

struct Stuff
{
    ParameterBlock<MoreStuff> more;
    Texture2D t;
};
ParameterBlock<Stuff> stuff;

When the compiler sees the declaration of stuff it "opens" a parameter block, which for Vulkan will get its own set=N. It then looks at fields inside the block, and sees the nested parameter block more, which gets assigned set=N+1, and then it sees the texture t which will get binding=0 inside set=N.

(Caveat: nesting of parameter blocks like this is intended to work, but hasn't been adopted by users, so I cannot promise that it won't break)

If the Stuff type had only contained nested parameter blocks, then the compiler would no longer allocate a full set just for the outer block, and instead would only allocate sets to the inner blocks (Slang never allocates empty sets just like it never allocates empty constant buffers).

With all that background, it is hopefully easy to see what Slang is doing to your overall shader. The right way to look at it is that Slang "opens" a fresh parameter block (for the "default" block) at the top of your file, and then scans your global declarations as the fields of that block. Ordinary resources/buffers/etc. become bindings within the default block, whereas nested ParameterBlocks get their own sets after the default block. At the end of scanning your code, if there was nothing in that default block, then Slang will optimize it out of existence, and everything after it will shift down in set number.

Unrelated Notes

From the examples you've been posting in your bug reports, it seems like you are mostly using declarations like:

ParameterBlock<X> myStuff;

as an alternative to ordinary constant buffer declarations:

cbuffer MyStuff
{
    X myStuff;
}

So far all of your examples have only put pure "uniform" data inside of ParameterBlocks, and you've been declaring your resources in the global scope as per traditional HLSL. It is fine to use Slang this way (I'm not here to tell you how to write your application), but I'd note that it isn't very idiomatic of either HLSL or Slang.

If you want to write something closer to idiomatic HLSL, you may want to just use ordinary cbuffer declarations, so that your original code becomes:

cbuffer U
{
    UBOData ubo;
}
SamplerState     textureSampler;
Texture2D        textureSource;

That code should work with any HLSL front-end, and not just Slang. Slang would put all of those declarations into the same set by default, which may or may not be what you want.

Alternatively, a more idiomatic Slang way of doing things would be to declare a single struct type that encapsulates all` the parameters of your effect:

struct MyParams
{
    /* ... contents of UBOData go here ... */
    SamplerState textureSampler;
    Texture2D textureSource;
};
ParameterBlock<MyParams> myParams;

In this second example we've encapsulated all the parameters of a single effect (which might be your top-level shader, or it might be some effect defined in a library you import) in a single struct type, which means we can pass it around a MyParams value to our subroutines, etc.

In this example all of the resources that get declared based on that ParameterBlock (a UBO, a texture, and a sampler) will be in the same set=0, and you are free to declare other parameter blocks that use their own sets.

The features of Slang are geared more toward this second approach, where you encapsulate together related shader parameters whether they are uniform data or resources, rather than the more traditional GLSL/HLSL style that tries to enforce a harder mental separation before resources and things that are "just data."

@tangent-vector
Copy link
Contributor

@vasumahesh1 Does my response answer your questions? If so, I'd like to close this issue.

If you think the behavior of the compiler should be different in cases like this, I'd be interested to hear what you'd like the rules to be.

@vasumahesh1
Copy link
Author

vasumahesh1 commented Oct 12, 2018

Oh sorry, was a bit MIA for a while.

Thanks for answering my doubts. I had a few other things to verify.

So say I wanted the Sampler and the Texture image on the same set but different from the set of the UBO.

  1. Then I would be assigning them as two individual parameter blocks?

    struct TexSampler {
      SamplerState textureSampler;
      Texture2D textureSource;
    }
    
    struct UBO {
      // UBO Data
    }
    
    ParameterBlock<UBO> u1;
    ParameterBlock<TexSampler> u2;

    u1 will get set 0 and u2 will get set 1, right? Or is it only for the case of nesting these parameter blocks? (Sorry didn't have the time for testing this out)


  1. I was actually implementing render passes in my renderer using Slang shaders. I ran into this issue where I had to explicitly specify sets. I'm not sure if this is a really good way of doing this.

    In the linked shader, there is no UBO. But the UBO by default takes set 0 (for the vertex shader). Since it is not present in the shader file itself (as it is not used), I had to explicitly say that the sampler & texture is in set 1.

    EDIT: An approach like this also seems very reasonable and avoid explicit descriptor sets! What are your thoughts on this?

    What would be a good way around this?

    • Do I put the two shaders in 1 file?
    • Do I explicitly specify the sets and binding (not a big fan of that)?

@vasumahesh1
Copy link
Author

EDIT: For Part 1

It does work.

import Common;

ParameterBlock<UBOData> ubo;
ParameterBlock<SamplerBlock> samplerBlock;

float4 main(PixelShaderInput input) : SV_Target
{
  float4 color = float4(samplerBlock.g_txDiffuse.Sample(samplerBlock.g_sampler, input.uv).rgb, 1.0);
  return color;
}

even though UBO doesn't get used, Slang assigns the SamplerBlock to set 1.

#line 25 0
layout(binding = 0, set = 1)
uniform sampler samplerBlock_g_sampler_0;


#line 8 1
layout(binding = 1, set = 1)
uniform texture2D samplerBlock_g_txDiffuse_0;

@tangent-vector
Copy link
Contributor

u1 will get set 0 and u2 will get set 1, right? Or is it only for the case of nesting these parameter blocks? (Sorry didn't have the time for testing this out)

For that example, it should, yes.

What would be a good way around this?

  • Do I put the two shaders in 1 file?
  • Do I explicitly specify the sets and binding (not a big fan of that)?

The big picture here is that Slang expects/needs to have a global view of your shader code.

If you compile entry points one at a time - a vertex shader here, and then a fragment shader there, etc. - then somewhat obviously Slang can't take the resource bindings in your vertex shader into account when compiling the fragment shader.

There are three main ways around this:

1. Put all the entry points that will be used together in one file

If you currently have TestZone.vs.slang and TestZone.ps.slang then it seems pretty easy to just merge those into TestZone.slang and have a single file define a single effect/pass. I'd call that the idiomatic way to do things in Slang, because even your file naming convention is showing that you have a single logical "module" of code, that is being artificially being split into multiple files out of a sense of responsibility to have just one entry point per file (this is perhaps the legacy of some of GLSL's unfortunate design choices).

2. Use separate files, but have one import another

It might be that option (1) isn't great for you, because you have something like a "default" vertex shader in DefaultVS.slang and you want to be able to use that with a bunch of different fragment entry points which are in their own files.

If you always know for a given fragment shader what VS it will use, you can just have the fragment shader file import the vertex shader file so that its declarations are in scope to the resource binding logic.

I would still advocate in that case that you compile the vertex and fragment shader together, rather than try to compile the VS once and then re-use that compiled VS with multiple fragment shaders. The overhead of trying to make that option work (and ensure the VS parameters get the same set in all your different compilations) is likely not going to be worth it. It is generally easiest to think of compiling whole "programs" that consist of one entry point for each stage you plan to use.

Using the Slang API, it should be easy to specify multiple entry points to compile in a single compile request. Doing this from the command-line may be harder, but I would not advocate using slangc in very serious production cases.

Also, at the point where you have multiple files you may find that it is easier to use Slang's reflection API to find out what set things got assigned, rather than strictly rely on re-running Slang's layout algorithm in your head.

3. Keep things in separate files, but compile them in one go.

The final option would be to just define the vertex and fragment shader in separate files, but then pass both of those files to slangc at the same time. The Slang compiler lets you specify multiple input source files in a single compile request, and it will take the resources from all files into account when doing layout.

Like option (2), you probably still want to compile both entry points at once, rather than compile the vertex and fragment shaders separately and then just hope they agree.

@tangent-vector
Copy link
Contributor

even though UBO doesn't get used ...

To be clear, one of the most important design decisions in Slang is that we never take into account whether a parameter gets used or not when doing layout. We think this is the right option, especially for applications that implement "hot reload" of shaders, because you don't want the location of a shader parameter to change when you comment out some random line of code and then hot-reload.

Some existing applications depend on compilers to eliminate all their unused parameters to make them fit within API/driver limits, and those applications might have a hard time porting to Slang.

@vasumahesh1
Copy link
Author

vasumahesh1 commented Oct 12, 2018

Put all the entry points that will be used together in one file

This is something I ended up with after trying some stuff out. The idea is now very clear that we need to expose the descriptors in the correct order for slang to understand all of the layout correctly.

because you don't want the location of a shader parameter to change when you comment out some random line of code and then hot-reload.

Yes that is very true.

I believe this clears out a lot of things. Many thanks. I will be trying out my D3D12 implementation in a few weeks.

You can close this issue. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:question questions or discussions
Projects
None yet
Development

No branches or pull requests

2 participants