spv-out: Decorate integer builtins as Flat in the spirv writer #2035
Conversation
This file contains 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
|
It does not fix the |
jimblandy
requested changes
Aug 31, 2022
src/back/spv/writer.rs
Outdated
Comment on lines
1175
to
1178
| let is_flat = match built_in { | ||
| Bi::Position { .. } => false, | ||
| _ => true, | ||
| }; |
There was a problem hiding this comment.
The actual rule from the Vulkan spec is:
- VUID-StandaloneSpirv-Flat-04744
Any variable with integer or double-precision floating-point type and with Input storage class in a fragment shader, must be decorated Flat
It's true that spirv-val doesn't complain about (say):
@fragment
fn fs_main(@location(0) index: u32) -> @location(0) vec4<f32> {
return vec4<f32>(0.0, 0.0, 0.0, 0.0);
}
but since the background story is that these validations are just being implemented piecemeal as somebody's interest leads them, I think we should assume the above might well stop passing validation at some point in the future.
Anyway, that is too many words to say: Let's implement the rule as expressed in the Vulkan spec and attach Decoration::Flat to everything whose type is Sint, Uint, or 64-bit Float.
nical
force-pushed
the
spv-flat-builtins
branch
2 times, most recently
from
547c2cc
to
6ab06fd
Compare
nical
changed the title
nical
force-pushed
the
spv-flat-builtins
branch
from
6ab06fd
to
390fcd9
Compare
nical
changed the title
jimblandy
approved these changes
Sep 1, 2022
jimblandy
force-pushed
the
spv-flat-builtins
branch
from
390fcd9
to
fb3d626
Compare
|
(I made the minor changes I requested.) |
jimblandy
added a commit
to jimblandy/naga
that referenced
this pull request
Sep 1, 2022
The Vulkan decoration rules require us to distinguish vertex shader inputs, fragment shader inputs, and everything else, so just pass the stage to `Writer::write_varying`. Together with the SPIRV storage class, this is sufficient to distinguish all the cases in a way that closely follows the spec language.
jimblandy
added a commit
to jimblandy/naga
that referenced
this pull request
Sep 1, 2022
The Vulkan decoration rules require us to distinguish vertex shader inputs, fragment shader inputs, and everything else, so just pass the stage to `Writer::write_varying`. Together with the SPIRV storage class, this is sufficient to distinguish all the cases in a way that closely follows the spec language.
jimblandy
added a commit
that referenced
this pull request
Sep 1, 2022
The Vulkan decoration rules require us to distinguish vertex shader inputs, fragment shader inputs, and everything else, so just pass the stage to `Writer::write_varying`. Together with the SPIRV storage class, this is sufficient to distinguish all the cases in a way that closely follows the spec language.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #2032
Take what I write in this PR with a whole handful off salt, I'm new enough to naga and spirv that there's a decent chance I am jumping to conclusions.
See the investigation in #2032. My uneducated guess is that spirv wants the integer input builtins such as
primitive_indexto have the flat decoration. So I added that in fairly naive way.This PR fixes the
primitive_index.wgsltest case and doesn't appear to cause regressions as far ascargo test --allcan tell.