KhronosGroup / SPIRV-Reflect

SPIRV-Reflect is a lightweight library that provides a C/C++ reflection API for SPIR-V shader bytecode in Vulkan applications.
Apache License 2.0
672 stars 147 forks source link

Promote member decorations to descriptor binding decorations #243

Closed panos-lunarg closed 6 months ago

panos-lunarg commented 8 months ago

This is basically a follow-up on #141. Decorations like readonly or writeonly are being parsed but they are only applied to descriptor block variables. This patch scans each descriptor's member decorations and those that are common to all members are promoted to the descriptor as well.

CLAassistant commented 8 months ago

CLA assistant check
All committers have signed the CLA.

panos-lunarg commented 7 months ago

With the intention of adding a test for this change I attempt to reuse the non_writtable_image.spv shader. Because decorations were not being tested for descriptor bindings I had to touch all .yaml files. Hope it is the right way

panos-lunarg commented 6 months ago

I assume you mean the following example:

layout(set=0, binding=0) buffer storage_buffer {
    writeonly float a; // non-readable
    readonly float b; // non-writable
} foo;

In the above example since none of the decorations are common between all member, none will be promoted to the whole descriptor. In order to get both readonly and writeonly promoted to the whole descriptor all member must have the decorations. Something like this:

layout(set=0, binding=0) buffer storage_buffer {
    readonly writeonly float a; // both non-readable and non-writable
    readonly writeonly float b; // both non-readable and non-writable
} foo;

This indeed will mark storage_buffer as both readonly and writeonly but if glslang allows you to mark a member with both then I don't see the problem in promoting both to the descriptor

spencer-lunarg commented 6 months ago

In order to get both readonly and writeonly promoted to the whole descriptor all member must have the decorations.

Awww, I see, yes, you are doing &= on these

Could you quick add my example as a test case (show casing this code works like we intend it to) then I think I'm convinced this is ready to go