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

MSL: Support a runtime array with dynamic offset in an argument buffer. #2347

Merged

Conversation

billhollings
Copy link
Contributor

  • Modify get_resource_array_size() to retrieve size of descriptors in an argument buffer from resource_bindings (reversing change made in b236352).
  • Retrieve size of runtime array from get_resource_array_size().

@HansKristian-Work This PR reverses the b236352 commit from PR #2329. It was not clear why the restriction from that commit is required (what is the ABI referenced there?), and it means that variables held within an argument buffer descriptor set cannot have their sizes established by the app. This is particularly troublesome for SPIR-V runtime arrays, whose size must be set by the app.

Several hundred CTS tests use a runtime array in SPIR-V to read from a fixed-size array defined in a Vulkan descriptor set.

If the restriction in b236352 is still required, perhaps this PR could add a parameter to msl_options to control when is it required.

spirv_msl.cpp Outdated Show resolved Hide resolved
- Retrieve size of runtime array from get_resource_array_size().
- Move validation assertion to after retrieval of array size.
Copy link
Contributor

@HansKristian-Work HansKristian-Work left a comment

Choose a reason for hiding this comment

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

LGTM

@HansKristian-Work HansKristian-Work merged commit 6fd1f75 into KhronosGroup:main Jun 19, 2024
6 checks passed
@billhollings billhollings deleted the arg-buff-runtime-array branch June 20, 2024 00:24
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.

2 participants