[SpecializationConstant] should not cause allocation of descriptor set binding.

[ov-l]

Hi,

First off, thanks for providing a shader playground.
I was hoping that this could be supported, where all push constants are grouped together into a single constant block

// Somehow multiple push_constant can be declared !!
// But they dont compile into valid glsl
[vk::push_constant]
ConstBufferPointer<MaterialData> gPushConstants;

[vk::push_constant]
ConstBufferPointer<ObjectData> gObjectData;

Instead it generates glsl with two push constant blocks. I believe at-least it should throw an error. Shader link

Thanks in advance.

[csyonghe]

The [vk::push_constant] attribute is provided for backward compatibility with DXC, which also does not allow multiple push constant blocks.

In Slang, the native way to use push constants is to define them as entrypoint uniform parameters.
All entrypoint uniform parameters are automatically translated to push constants, and they will be merged together too.

For example:

void main(uniform MaterialData* material, uniform ObjectData* object) {}

will result in SPIRV that puts both material and object into a single push constant block.

See playground demo here.

[ov-l]

Thanks that works for me.
One question, I am trying to come up with a way I can ensure a shader can be compiled in bindless and bindful path as per engine mode. Your suggestion would work great for me, as I could put all required parameters in a shader as function parameter. However, if I do not explicitly provide a binding index using vk::binding, why does the set start at 1: Shader

Also I am unsure if the typealias with generic is valid, but it seems to be compiling fine, so I guess it is fine ?

Thanks a lot for the quick reply. Lastly, would you mind giving your opinion if this is a good approach to handle scenario where both bindless and bindful path need to be supported by a shader or is there a better way to do it with slang.

Thanks in advance.

[csyonghe]

I think it is a bug for the ParameterBlock to be assigned set 1. This is likely due to the existence of:

[SpecializationConstant]
const int HasTrueSDF = 0;
[SpecializationConstant]
const int PassFilter = 0;

Slang might be thinking these are normal uniforms and need a uniform buffer binding in set 0, so any future parmaeter blocks got assigned set 1. Removing these the material block is now at set 0.

Slang currently does not have a first-class way of doing bindless, so I think your solution should work fine.

[ov-l]

Thanks, if you are okay, I keep this issue open to track the bug related to set assignment, or I could open a new issue, as you want.

[csyonghe]

We can keep this issue as is, I updated the title to reflect the issue.

[ov-l]

Thanks for the patch, however, I am still having issues with a generated shader after applying your patch. The SPIR-V output still contains DescriptorSet index as 1 as the only descriptor set. I will share with you the shader I am using and the SPIR-V generated. Shader /
SPIR-V

EDIT: Note that I tested without the Specialization constants, and set number and bindings are correct when I dont use them.
EDIT2: I think the issue probably comes from slang-emit-spirv, because the reflection code I have reports the descriptor space index as 0. However, emitOpDecorateDescriptorSet gets index value as 1 as its first index.

[csyonghe]

The playground hasn’t been updated to use top of tree slang. I updated it just now and it is now using set 0 with your shader code.

[ov-l]

My apologies. Indeed, I pulled the latest changes again and clean rebuild everything. Seems to be working after that.

[ov-l]

In-fact the issue I was having is not due to SpecializationConstants. I still end up having a descriptor set at index 1. Updated shader

The shader I have should not produce a new DescriptorSet. I was actually having the issue in the GraphicsPipeline creation which was reporting I have no layout in my descriptorSetLayout for DescriptorSet 1, and it had me confused why was there a set at 1 to begin with, because indeed the reflection interface was also not reporting it.

EDIT: The issue is probably because the texture is not in use in vertex shader, so when I link the two shaders, it computes set numbers separately for the ubo block and for the texture. Is there a way to force not to remove unused parameters or force all parameter group elements to be in a single set?

[csyonghe]

I think this is due to a bug when compiling both entry points into the same spirv module.

What happens if you compile just the fragment shader EntryPoint?

slangc shader.slang -target spirv -entry fragmentMain -stage fragment

[csyonghe]

It feels like the entry point parameters some how got duplicated when there are two entry points.

[ov-l]

In-fact if I just compile using shader playground for example, by removing either of the entry points, everything is good.

[ov-l]

It feels like the entry point parameters some how got duplicated when there are two entry points.

Another possibility is the TypeLayout is different due to parameter usage, so they are split into two different ParameterGroup. -> removing the texture declaration does not solve the issue, so I you are right, it just splits it into two groups and there is no matching between parameter type between different linked moduls

[pdjonov]

The [vk::push_constant] attribute is provided for backward compatibility with DXC, which also does not allow multiple push constant blocks.

In Slang, the native way to use push constants is to define them as entrypoint uniform parameters. All entrypoint uniform parameters are automatically translated to push constants, and they will be merged together too.

You may be thinking of [vk:push_constant] as a back-compat feature, but to me it's actually great way to pull repetitive code that deals with push constant data out of multiple entry point declarations across several files and into a module. Please don't ever deprecate it.

[csyonghe]

After looking more into this issue, I think we should keep the existing behavior.

Consider the following:

[shader("vertex")]
VertexOut vertexShader(uniform ParameterBlock<VertexParams> pv) {...}

[shader("fragment")]
float4 fragmentShader(uniform ParameterBlock<FragmentParams> pf, VertexOut vin) {...}

When compiling both shader entrypoints into the same spirv module, we have to put pv and pf into separate descriptor sets, because it is possible that vertexShader and fragmentShader are being included in the same graphics pipeline, in which case pv and pf must be bound to different descriptor sets.

So the current behavior that you are seeing is actually correct. When you have two entrypoints each declaring a ParameterBlock, the compiled spirv module will contain duplicate parameter declarations in different descriptor sets. If you wish to share the parameter binding across different entrypoints, the only solution right now is to declare them in the global scope.

For this reason, I am going to close this issue as won't fix.

[obhi-d]

Why is it not possible to match type-name between the two uniform passed to each method ? Does the set/binding assignment happen before linking the two shaders ? if not, I feel like it should be manageable to do the assignment once, if this info is stored in the AST. But anyway, if this is a permanent restriction, I don't see function parameters on entry points as very useful.

[csyonghe]

This kind of matching and deduplication is too much magic and may lead to surprises. It is completely possible and valid for different EntryPoint parameters of the same type to mean different instances. In general the compiler shouldn't deduplicate two distinct declarations just because they have the same type.

As said in previous post, if sharing of a parameter among different entry points is intended, then declaring it in global scope is the only viable way currently.