-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[naga] Add GLSL constant_id layout qualifier support and WGSL backend override output #8589
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
Conversation
ef924a2 to
35ab6a0
Compare
916411a to
291adc7
Compare
naga/src/proc/constant_evaluator.rs
Outdated
| Behavior::Glsl(_) => { | ||
| unreachable!() | ||
| // GLSL specialization constants (constant_id) become Override expressions | ||
| Behavior::Glsl(GlslRestrictions::Const | GlslRestrictions::Runtime(_)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, GL_KHR_vulkan_glsl allows specialization constants to be used in constants:
layout(constant_id = 18) const int scX = 1;
layout(constant_id = 19) const int scZ = 1;
const ivec3 scVec = ivec3(scX, 1, scZ); // partially specialized vectorBut does this work, would you be able to add a test for this?
I think even if we allow overrides in GlslRestrictions::Const here it won't since the validator will complain that constants use overrides in their initializers.
The GLSL frontend would have to emit overrides for all constants that (indirectly) use specialization constants.
We can of course put this off and create an issue for it, but I was just wondering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I've added the test case and you're right - it doesn't work correctly. The generated WGSL hardcodes the constant_id values (18, 19) instead of referencing the overrides.
const scVec: vec3<i32> = vec3<i32>(18i, 1i, 19i); Fixing this would require the GLSL frontend to track transitive dependencies on specialization constants. I think we should keep this PR focused on direct specialization constant usage (which works) and create a separate issue for this case.
Sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds fine but it seems it's misbehaving rather than it being a validation issue, can you try doing this for the const path so that we get an error instead (for now):
Behavior::Glsl(GlslRestrictions::Const(_)) => {
Err(ConstantEvaluatorError::OverrideExpr)
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
teoxoy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good! Just left a comment.
c78617b to
7f3c041
Compare
7f3c041 to
12e47af
Compare

Connections
No dependencies on other PRs. Please add related issues if any.
Description
This PR adds support for GLSL specialization constants (
constant_idlayout qualifier) in the Naga GLSL frontend and enables the WGSL backend to outputoverridedeclarations.layout(constant_id = N)qualifier forconstdeclarationsOverrideinstead ofConstantOverrideexpressions in the constant evaluatorwrite_override()method to output@id(N) overridedeclarationsExpression::Overridein expression writerswrite_overrideinTypeContextNameKey::Overridevariant for override namingTesting
spec-constant.fragtest case with various specialization constant typesSquash or Rebase?
This branch contains a single functional commit and can be rebased directly.
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.