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

Promote member decorations to descriptor binding decorations #243

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

panos-lunarg
Copy link
Contributor

@panos-lunarg panos-lunarg commented Jan 9, 2024

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
Copy link

CLAassistant commented Jan 9, 2024

CLA assistant check
All committers have signed the CLA.

if (IsNotNull(p_type_node) && p_type_node->member_count) {
SpvReflectPrvDecorations common_flags = p_type_node->member_decorations[0];

for (uint32_t m = 1; m < p_type_node->member_count; ++m) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was curious why this starts at 1 intead of 0?

Copy link
Contributor Author

@panos-lunarg panos-lunarg Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Member decoration at 0 is read in the line above. Then the rest are logical &ed (edit: well this is the bitwise operator but the variables are booleans) one at a time. Result is if a decoration is true for all members, in the end it will remain true. These will be promoted to the whole descriptor. If there is only one decoration (at 0) then its decorations will be promoted to the whole descriptor.

@panos-lunarg
Copy link
Contributor Author

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

Copy link
Contributor

@spencer-lunarg spencer-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, the tests (that aren't NONE) make sense to me looking at it

Copy link
Contributor

@spencer-lunarg spencer-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After taking a second look at this, I am not sure if this is the correct move, if you have a shader like

layout(set=0, binding=0) buffer storage_buffer {
    float a; // non-readable
    float b; // non-writable
} foo;
               OpCapability Shader
               OpMemoryModel Logical GLSL450
               OpEntryPoint GLCompute %main "main"
               OpExecutionMode %main LocalSize 1 1 1
               OpMemberDecorate %storage_buffer 0 Offset 0
               OpMemberDecorate %storage_buffer 1 Offset 4
               OpMemberDecorate %storage_buffer 0 NonReadable
               OpMemberDecorate %storage_buffer 1 NonWritable
               OpDecorate %storage_buffer BufferBlock
               OpDecorate %foo DescriptorSet 0
               OpDecorate %foo Binding 0
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %float = OpTypeFloat 32
%storage_buffer = OpTypeStruct %float %float
%_ptr_Uniform_storage_buffer = OpTypePointer Uniform %storage_buffer
        %foo = OpVariable %_ptr_Uniform_storage_buffer Uniform
       %main = OpFunction %void None %3
          %5 = OpLabel
               OpReturn
               OpFunctionEnd

I don't think the desired result for spirv-reflect is to have the whole descriptor marked as both NonReadable and NonWriteable

@panos-lunarg
Copy link
Contributor Author

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
Copy link
Contributor

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

binding: 0
input_attachment_index: 0
set: 0
decorations: 0x00000000 # NONE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, as expected!

Decoration like non-writable and non-readable are found in spirv as
member decorations. This patch detects decorations that are shared
between all members and promotes them into descriptor binding
decorations.
@spencer-lunarg spencer-lunarg merged commit 8406f76 into KhronosGroup:main Feb 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants