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 SPIR-V validation of image ops with offsets #2575

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

LDeakin
Copy link
Contributor

@LDeakin LDeakin commented Oct 14, 2024

Shader functions like textureLodOffset would incorrectly fail validation.

Checks like this would always evaluate to true (and fail validation) because the property wraps:

if offset < properties.min_texel_offset as u64 {

Changelog:

### Bugs fixed
- Fix SPIR-V validation of image ops with offsets

Shader instructions like `textureLodOffset` would always fail validation.
@LDeakin LDeakin changed the title Fix SPIR-V validation of image op functions with offsets Fix SPIR-V validation of image ops with offsets Oct 14, 2024
@LDeakin LDeakin force-pushed the fix_negative_image_op_offsets branch from 1d88d39 to 69a66db Compare October 14, 2024 06:57
@Rua
Copy link
Contributor

Rua commented Oct 15, 2024

Thanks! One remark: The get_constant_* functions are supposed to return the constants as unsigned integers. If they should return signed integers, they should be renamed to get_constant_signed_* so that they match get_constant_float_*

@LDeakin
Copy link
Contributor Author

LDeakin commented Oct 18, 2024

I've renamed the offending functions. Note that now there are no unsigned variants of get_constant_composite_composite and get_constant_maybe_composite, but they would be dead code if I brought them back. Thoughts?

@marc0246
Copy link
Contributor

I'm pretty sure this is what Rua meant. We will add those functions back if/when they are needed. Thank you for this ❤️

@marc0246 marc0246 merged commit 123eda1 into vulkano-rs:master Oct 18, 2024
6 checks passed
@marc0246
Copy link
Contributor

marc0246 commented Oct 18, 2024

A sidenote: this validation was never released. There isn't going to be a changelog entry.

@LDeakin LDeakin deleted the fix_negative_image_op_offsets branch October 18, 2024 06:17
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.

Image offsets are compared with the min_texel_gather_offset device property as u64 instead of i32.
3 participants