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

[spv-out] decorate non-uniform image/sampler access chains #4766

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Nov 24, 2023

@teoxoy teoxoy requested a review from a team as a code owner November 24, 2023 11:32
@Elabajaba
Copy link
Contributor

Tested this and it fixed the issue on windows+amd+vulkan for me, and didn't regress linux (radv)+amd+vulkan.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

I'm confused - the patch doesn't match the commit message or the discussion.

The code looks like it has always been decorating the results of OpAccessChain, just not for binding arrays in the Handle address space.

The discussion suggests that both the pointer and the result of the load need to be decorated, but we are not decorating the result of the load / sample at all, as far as I can see.

Can't we do better at helping ourselves understand what's going on the next time we have to look at this?

@teoxoy
Copy link
Member Author

teoxoy commented Nov 27, 2023

The code looks like it has always been decorating the results of OpAccessChain, just not for binding arrays in the Handle address space.

The code originally decorated all access chains, c3e35df changed it so that it only happened for storage and uniform buffers since I thought that decorating the image/sampler access chains was not necessary. We now know this was necessary and is what this PR is reverting.

The discussion suggests that both the pointer and the result of the load need to be decorated, but we are not decorating the result of the load / sample at all, as far as I can see.

Since c3e35df we decorate the load result of image/sampler binding arrays (the load result being an image/sampler); this is part of the VUID requirement.

The results of texture load/sampling operations don't have to be decorated.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Okay, I understand this better now. But please update the commit message.

@teoxoy
Copy link
Member Author

teoxoy commented Dec 4, 2023

How should the commit message be updated?

@jimblandy
Copy link
Member

@teoxoy Here's my suggestion (summary line and detailed explanation):

[spv-out] Decorate all non-uniform binding array access chains as NonUniform, not just buffer binding arrays.

Apply the NonUniform decoration to the results of all access chains rooted in binding arrays that use non-uniform values as indices, regardless of the binding array's element type and address space. Previously, Naga only decorated non-uniform access chains for binding arrays of buffers.

The important difference is that this message doesn't mention load results, which this PR has no effect on (and which Naga has never decorated as far as I can tell).

…nUniform`, not just buffer binding arrays.

Apply the `NonUniform` decoration to the results of all access chains rooted in binding arrays that use non-uniform values as indices, regardless of the binding array's element type and address space. Previously, Naga only decorated non-uniform access chains for binding arrays of buffers.
@teoxoy teoxoy enabled auto-merge (rebase) December 7, 2023 17:51
@teoxoy teoxoy merged commit 411c1e5 into gfx-rs:trunk Dec 7, 2023
27 checks passed
@teoxoy teoxoy deleted the fix-decoration branch December 7, 2023 18:01
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.

Texture sampling issues when using a texture binding_array on Windows+AMD gpu with wgpu 0.18
3 participants