Skip to content

Rendering: SPIR-V reflection incorrectly determined writable state #103614

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stuartcarnie
Copy link
Contributor

Identified from testing #101696

Resolves a bug in RenderingDeviceDriver::_reflect_spirv:

if (may_be_writable) {
uniform.writable = !(binding.type_description->decoration_flags & SPV_REFLECT_DECORATION_NON_WRITABLE) && !(binding.block.decoration_flags & SPV_REFLECT_DECORATION_NON_WRITABLE);
} else {
uniform.writable = false;
}

When reflecting an image type, the binding.block struct is invalid, as that only applies to struct types. Here is the dumped value of the binding variable from the debugger, abbreviated:

{
  spirv_id = 65
  name = 0x000000011c28fd80 "current_image"
  binding = 0
  input_attachment_index = 0
  set = 0
  descriptor_type = SPV_REFLECT_DESCRIPTOR_TYPE_STORAGE_IMAGE
  resource_type = SPV_REFLECT_RESOURCE_FLAG_UAV
  image = (dim = SpvDim2D, depth = 0, arrayed = 0, ms = 0, sampled = 2, image_format = SpvImageFormatR32f)
  block = {
    spirv_id = 0
    name = 0x0000000000000000
    offset = 0
    absolute_offset = 0
    size = 0
    padded_size = 0
    decoration_flags = 0
    flags = 0
    member_count = 0
    members = nullptr
    type_description = nullptr
    word_offset = (offset = 0)
  }
  count = 1
  accessed = 1
  uav_counter_id = 4294967295
  uav_counter_binding = nullptr
  byte_address_buffer_offset_count = 0
  byte_address_buffer_offsets = 0x0000000000000000
  type_description = 0x000000011c295470
  word_offset = (binding = 175, set = 171)
  decoration_flags = 128
  user_type = SPV_REFLECT_USER_TYPE_INVALID
}

Note that the the entire binding.block struct is 0, which means this will evaluate to true, as the second part of the expression does not have the SPV_REFLECT_DECORATION_NON_WRITABLE bit set, as it is invalid and always 0:

uniform.writable = !(binding.type_description->decoration_flags & SPV_REFLECT_DECORATION_NON_WRITABLE) && !(binding.block.decoration_flags & SPV_REFLECT_DECORATION_NON_WRITABLE);

Metal has its own implementation to reflect the SPIR-V, which distinguishes the types:

if (a_type.basetype == BT::Struct) {
Bitset flags = compiler.get_buffer_block_flags(res.id);
uniform.writable = !compiler.has_decoration(res.id, spv::DecorationNonWritable) && !flags.get(spv::DecorationNonWritable);
} else if (a_type.basetype == BT::Image) {
if (a_type.image.access == spv::AccessQualifierMax) {
uniform.writable = !compiler.has_decoration(res.id, spv::DecorationNonWritable);
} else {
uniform.writable = a_type.image.access != spv::AccessQualifierReadOnly;
}
}

Note

SPIR-V cross throws an exception if you try to fetch the block metadata for an image.

Verified

This commit was signed with the committer’s verified signature.
ljharb Jordan Harband
@stuartcarnie stuartcarnie requested a review from a team as a code owner March 5, 2025 07:00
@AThousandShips AThousandShips added bug topic:rendering cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release labels Mar 5, 2025
@AThousandShips AThousandShips added this to the 4.5 milestone Mar 5, 2025
stuartcarnie added a commit to stuartcarnie/godot-demo-projects that referenced this pull request Mar 5, 2025
Previously,

- set 0 and 1 were read-only, and
- set 1 was write-only

Metal's SPIR-V reflection implementation correctly distinguished these
attributes, and resulted in the failure described by issue
godotengine/godot#101696

A fix for Godot's default SPIR-V reflection is found in godotengine/godot#103614

To ensure the demo keeps working, we ensure all sets have matching
formats.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release topic:rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants