Multiple push constants overlap.

[andriyDev]

Description
A shader can specify multiple push constant variables. These variables overlap their memory layout.

Repro steps
I created an example + test of the issue. I have written the example to work with the current version to show the oddity.
https://github.com/andriyDev/wgpu/tree/multiple-push-constants

Expected vs observed behavior
Expected behaviour: Either wgpu rejects the shader (because it has more than two push_constant variables), or push_constant variables are stored in order in push_constant memory.
Observed behaviour: wgpu accepts the shader and overlaps the memory layout of both variables. This means push constants structs require storing padding in order for structs to not overlap.

Platform
OS: Windows
wgpu version: HEAD - SHA 5c2c840 - 0.17.0
Backend: vulkan, opengl, dx12 (probably all of them).

[teoxoy]

Thanks for the detailed report! I think we are missing some validation.

[andriyDev]

I'll try to look into that!

What should the behaviour be for this case though? Do we want to support multiple push constants, or just call it invalid? I ask because I'm not confident with my understanding of push constants yet, like PushConstantRange don't really make sense to me haha.

[teoxoy]

The docs of PushConstantRange.stages indicate that you can only have one push constant per entry point.

Stage push constant range is visible from. Each stage can only be served by at most one range. One range can serve multiple stages however.

[andriyDev]

That sounds to me like you'd want multiple push constant vars to be disjoint, that way you could have separate push constant vars for each stage?

[teoxoy]

I don't think it matters if you use the same push constant var for multiple entry points/stages.

You can have multiple push constant vars in your wgsl source but only one might be used per entry point/stage. We already have this kind of tracking in naga (tracking which global variables are used by which entry points). So I think the only thing to do is to validate that no more than one push constant var is used by an entry point.

[andriyDev]

Well here's the issue (if I understand it correctly), since push constants all overlap, they actually correspond to the same range. So if we want one push constant var per stage, we still need to solve this "offset" problem.

But yes, I agree that using the same push constant var for multiple entry point/stages should be totally fine.

[teoxoy]

The docs of RenderPass.set_push_constants have some more info.

Even though the push constant storage is "shared" between stages the API will take care of correctly writing the data you passed to set_push_constants for each stage.

I see one issue though. Take the example given in the docs linked to above; bytes 0..8 will be visible by the fragment stage and bytes 4..12 by the vertex stage. How should the struct look like for the push constant var for the vertex stage? I think right now you'd have to manually pad it by one 32bit value to get to the data you want at bytes 4..12.

What happens if you try to read from the padding? Vulkan for one says you get an undefined value.

I imagine we'd want to add an offset to every member of the struct behind the scenes to avoid this issue and allow users to specify only the data they intended to get.

@cwfitzgerald what do you think?

[andriyDev]

Yes precisely! If we were to do that, I think checking that one entry point has one push constant var is no longer necessary. Since the bytes won't overlap between push constant vars (provided that your per-stage push constants are adjacent in memory), it's fine to use them all!

[teoxoy]

I think we would still need to make sure that one entry point uses at most one push constant since it won't be clear to the shader author how multiple push constants will be ordered. I guess declaration order would work but it's not obvious.

[andriyDev]

I would argue that declaration order is intuitive and already kinda part of wgsl - the alignment of structs in wgsl is INCREDIBLY order dependent (and getting the alignment wrong results in very wild behaviour). But I think one push constant per entry point is also fine.

Also, I've implemented this in naga which I think is the correct place for this.

(also sorry for the delay haha)

[teoxoy]

With gfx-rs/naga#2484 merged there is still one last issue: #4099 (comment).

[teoxoy]

Talked with @cwfitzgerald and he agreed with the concern raised in #4099 (comment).

Filed #4502 to track progress on that front.