spirv-opt: Adding deduplication of NonWritable OpLoads#6413
Open
alister-chowdhury wants to merge 9 commits intoKhronosGroup:mainfrom
Open
spirv-opt: Adding deduplication of NonWritable OpLoads#6413alister-chowdhury wants to merge 9 commits intoKhronosGroup:mainfrom
alister-chowdhury wants to merge 9 commits intoKhronosGroup:mainfrom
Conversation
alister-chowdhury
changed the title
s-perron
reviewed
Nov 28, 2025
dneto0
reviewed
Dec 10, 2025
Collaborator
dneto0
left a comment
dneto0
left a comment
There was a problem hiding this comment.
The tests look wrong. NonWritable is not put onto the struct type.
It's put on the OpVariable or the member of the struct.
dneto0
requested changes
Dec 10, 2025
Collaborator
dneto0
left a comment
dneto0
left a comment
There was a problem hiding this comment.
The implementation and testing look wrong. See above.
|
|
dneto0
approved these changes
Mar 27, 2026
s-perron
approved these changes
Mar 27, 2026
The current logic would only mark a load as read-only if either the OpVariable had NonWritable or a Uniform was not a storage buffer. This adds `IsVulkanReadOnlyStorageBuffer`, which does mostly the same stuff as `IsVulkanStorageBuffer`, with the addition of testing for NonWritable. When used as part of `IsReadOnlyLoad` for assigning value numbers, prevents it from being forced a unique value. Fixes KhronosGroup#6412
…alled by IsVulkanReadOnlyStorageBuffer
* Renaming `IsVulkanReadOnlyStorageBuffer` to `IsVulkanStorageBufferNonWriteable` Mainly to not accidentally mislead someone who comes across it. * Changed `IsReadOnlyPointerShaders` to return an enum, that can provide a hint that we may need to check other variables. * Added `IsSetBindingUniformlyReadOnly` which is basically for said hint, to make sure nothing that isn't read-only is bound to the same address.
* Changed IsVulkanStorageBufferNonWritable to test members, for NonWritable. * Specified IsSetBindingUniformlyReadOnly should be called after IsReadOnlyPointerShaders returns true. * Updated tests to use OpMemberDecorate. * Updated tests to include testing for having a NonWritable member.
s-perron
force-pushed
the
deduplicating_loads
branch
from
a1ac686 to
2125eb5
Compare
Collaborator
|
I rebased on main, and fixed the formatting. See if that affects the tests. |
Collaborator
|
This seems to have caused a real problem for the fuzzer test. @alister-chowdhury Let me know if you need help with that. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current logic would only mark a load as read-only if either the OpVariable had NonWritable or a Uniform was not a storage buffer.
This adds
IsVulkanReadOnlyStorageBuffer, which does mostly the same stuff asIsVulkanStorageBuffer, with the addition of testing for NonWritable.When used as part of
IsReadOnlyLoadfor assigning value numbers, prevents it from being forced a unique value.Fixes #6412