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

Fix promoted extension dependencies in vk.xml #2492

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

CodingRays
Copy link
Contributor

Fixes #2491

@CLAassistant
Copy link

CLAassistant commented Feb 4, 2025

CLA assistant check
All committers have signed the CLA.

@oddhack
Copy link
Contributor

oddhack commented Feb 5, 2025

Two comments on superficial consistency review (mostly for @spencer-lunarg who signed up as the other reviewer):

  • VK_KHR_shader_quad_control - VK_KHR_vulkan_memory_model was promoted to 1.2, but the vulkanMemoryModel capability was made optional
  • VK_EXT_device_generated_commands - VK_KHR_buffer_device_address was promoted to 1.2, but the bufferDeviceAddress feature capability was made optional

Possibly this would affect use of these extensions when 1.2 is supported but the corresponding promoted extension was not.

@oddhack
Copy link
Contributor

oddhack commented Feb 5, 2025

@asuessenbach this is getting a Vulkan-Hpp CI failure, but it appears more likely to be a syntax error in the generated code than a problem with this PR - could you take a look?

@oddhack
Copy link
Contributor

oddhack commented Feb 5, 2025

Followup: we think that the two partial promotions above should not be reflected in the XML, because they basically make the entire functionality of the extension optional if only 1.2 is supported. @CodingRays could you make those reversions?

Copy link

@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.

ran through Validation code gen, didn't have any issues, so tooling PoV - LGTM

@CodingRays
Copy link
Contributor Author

@oddhack VK_KHR_buffer_device_address is used in this way in other dependencies.
For example VK_NV_device_generated_commands has the following dependency (VK_VERSION_1_1+VK_KHR_buffer_device_address),VK_VERSION_1_2

Also found it in VK_KHR_acceleration_structure, VK_EXT_descriptor_buffer, VK_NV_copy_memory_indirect and VK_NV_memory_decompression.

@oddhack
Copy link
Contributor

oddhack commented Feb 19, 2025

We discussed and you are correct about the other cases. We think the path forward will be to document the feature restrictions in VU statements since capturing it in the XML is not currently possible. So, signing this off, but we'll have to sort out the CI failure - I think Vulkan-Hpp may have a bug which may or may not have been fixed, so will try re-running it to see if that helps.

@oddhack oddhack added this to the Signed-off to Merge milestone Feb 19, 2025
@oddhack oddhack self-assigned this Feb 19, 2025
@oddhack
Copy link
Contributor

oddhack commented Feb 21, 2025

I'm not sure what's up with the hpp-compile CI stage but it's not specific to this PR, so accepting it. Guessing that the underlying image is running an out of date g++ or something like that, as it's affecting other branches as well.

@oddhack oddhack merged commit 80128cc into KhronosGroup:main Feb 21, 2025
9 of 10 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.

Inconsistent extension dependencies using promoted extensions
4 participants