[Feature Request] [SPIR-V] Allow more than one push constant struct in Library shaders

[keptsecret]

Is your feature request related to a problem? Please describe.
In SPIR-V, we actually are able to have more than one push constant struct in a shader with multiple entrypoints. See: https://godbolt.org/z/ovj17dKro.
This is currently not possible when compiling SPIR-V using DXC. We get the error error: cannot have more than one push constant block. See: https://godbolt.org/z/69GW8KdfW

Describe the solution you'd like
Ideally, to be able to include two push constant structs and have the unused stuff be optimized out later on. Like in the second godbolt sample.

[[vk::push_constant]] PC0 pc0;

[numthreads(1,1,1)]
[shader("compute")]
void main0()
{
    // use only pc0 here
}

[[vk::push_constant]] PC1 pc1;

[numthreads(1,1,1)]
[shader("compute")]
void main1()
{
    // use only pc1 here
}

Describe alternatives you've considered
Current workaround is to put the two push constant structs in one big one and update with push constant offsets.
However, that comes with its own limitations.

[devshgraphicsprogramming]

One more thing to add, we usually don't use more than one push constant per pipeline because we Aggressively DCE the SPIR-V module and strip away entry points not used.

[Keenuts]

Hi!

I'm not sure this is something that we can easily fixed in DXC:

• Vulkan requires 1 PushConstant interface per entrypoint.
• inlining+DCE is done in SPIRV-Tools, after all codegen/sema checks.

This means in DXC, we only see each functions independently, and don't have a clear map of which entrypoint ends up using which push constant, so it is assumed each entrypoint is using every push constant we find.
Because we do this AST->SPIR-V generation, it's not easy do to anything else. A solution would be to let DXC generate those bad entrypoints, and in SPIRV-Tools, after inline etc, cleanup the unused push constants and hope we end up with a single PC per entrypoint. If not (bad HLSL for ex), we'd have a not-so helpful message saying Entry point id 'X' uses more than one PushConstant interface. which is not great.

I think this is something we can fix in Clang as we have a proper IR step to do this kind of analysis, but I'm not sure this is a priority for DXC.

@s-perron if you disagree?

[devshgraphicsprogramming]

Hi!

I'm not sure this is something that we can easily fixed in DXC:

* Vulkan requires 1 PushConstant interface per entrypoint.

Yes and we have multiple entry points, if you look at our repro/example you'll see that only one Push Constant struct is used per entry point Static Call graph

* inlining+DCE is done in SPIRV-Tools, after all codegen/sema checks.

This means in DXC, we only see each functions independently, and don't have a clear map of which entrypoint ends up using which push constant, so it is assumed each entrypoint is using every push constant we find.

SPIRV-Tools which you use for legalization and validation actually do.

Its right here

               OpEntryPoint GLCompute %1 "main0" %2
               OpEntryPoint GLCompute %3 "main1" %4

The %2 and %4 are the global push constant OpVariable declarations that the entry point is using

          %2 = OpVariable %_ptr_PushConstant__struct_5 PushConstant
          %4 = OpVariable %_ptr_PushConstant__struct_6 PushConstant

If we tried to use more than 1, you'd see something like

               OpEntryPoint GLCompute %main0 "main0" %pc0
               OpEntryPoint GLCompute %main1 "main1" %pc1 %pc0

Which the current spirv-val and spirv-opt don't catch, yet.

Because we do this AST->SPIR-V generation, it's not easy do to anything else. A solution would be to let DXC generate those bad entrypoints, and in SPIRV-Tools, after inline etc, cleanup the unused push constants and hope we end up with a single PC per entrypoint. If not (bad HLSL for ex), we'd have a not-so helpful message saying Entry point id 'X' uses more than one PushConstant interface. which is not great.

Isn't DCE already a part of the required legalization passes, or am I wrong here ?

[Keenuts]

It seems optimization is not always guaranteed, see https://godbolt.org/z/5YM6Goj44
%pc0 is present on both EntryPoints, even if the second function is not using this.

And the validator does catch if the two '%pc` are in the same interface definition: https://godbolt.org/z/rxz941xs5

DCE and all is part of the legalization. But as you can see, it doesn't always cleanup unused PC from the interface instruction, meaning validation will fail.

We could imagine fixing SPIR-V tools to correctly clean this up, but now it means someone using normal HLSL, if they use 2 push constants in the same entrypoint, will see the unhelpful DXC message saying:
fatal error: generated SPIR-V is invalid because blablabla

[s-perron]

I think we can go the legalization route. It should be no different than other resources. What we need to do is set the "needs legalization" bit when there are multiple push constants. Or just do legalization all of the time.

As usual, legalization is on a best-effort basis. If the generated spirv is invalid, so be it.

That is my suggestion.