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] Fix non-uniformity attributes #2421

Closed
wants to merge 1 commit into from

Conversation

cwfitzgerald
Copy link
Member

Fixes #2408

This isn't the cleanest change, but I couldn't really figure out a better way to structure it.

@teoxoy
Copy link
Member

teoxoy commented Aug 4, 2023

There seems to be a lot of uncertainty around where the NonUniform decoration is required (KhronosGroup/SPIRV-Registry#89, KhronosGroup/Vulkan-ValidationLayers#3288 (comment)).

The SPIR-V Spec on NonUniform doesn't say much.

Apply only to an object. Asserts that the value backing the decorated is not dynamically uniform. See the client API specification for more detail.

Object: An instantiation of a non-void type, either as the Result of an operation, or created through OpVariable.

The SPIR-V Spec on Uniformity says it should be present on image operands of image operations otherwise they are assumed to be uniform.

An is defined to be dynamically uniform for a dynamic instance of an instruction if all invocations (in an invocation group) that execute the dynamic instance have the same value for that . This is not something that is explicitly decorated, it is just a property that arises. This property is assumed to hold for operands of certain instructions, such as the Image operand of image instructions, unless that operand is decorated as NonUniform. Some implementations require more complex instruction expansions to handle non-dynamically uniform values in certain instructions, and thus it is mandatory for certain operands to be decorated as NonUniform if they are not guaranteed to be dynamically uniform.

The VUID-RuntimeSpirv-NonUniform-06274 is more insightful, although I had to read it a few times to understand what (I think) it's trying to say. That is:

  • image operands (for image operations) need the decoration; which matches what the SPIR-V spec was saying in the uniformity section; in our case the result of the OpLoad
  • pointer operands (for load, store and atomic operations) need the decoration; in our case the result of OpAccessChain

If an instruction loads from or stores to a resource (including atomics
and image instructions) and the resource descriptor being accessed is
not dynamically uniform, then the operand corresponding to that resource
(e.g. the pointer or sampled image operand) must: be decorated with
NonUniform

I've also looked at glslang's source code and it matches my understanding.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Besides the comment I left, the PR looks good but I think we can take a simpler approach (see #2422).

OpDecorate %55 NonUniform
OpDecorate %58 NonUniform
Copy link
Member

Choose a reason for hiding this comment

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

I think for buffers we need to keep the decoration on the access chain.

@cwfitzgerald
Copy link
Member Author

Well that's not needlessly complicated...

Thanks for doing that research!

@teoxoy
Copy link
Member

teoxoy commented Aug 4, 2023

It was quite the rabbit hole 😅

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.

Artifacts when using binding_array with textures. #2282 Related
2 participants