-
Notifications
You must be signed in to change notification settings - Fork 193
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
Implement invariant attribute #1789
Conversation
BuiltIn { | ||
built_in: BuiltIn, | ||
/// can only be `true` for [`BuiltIn::Position`] | ||
invariant: bool, |
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.
Perhaps there is a better place for this flag to be?
If user locations are to be supported in the future than the current API of this PR wouldn't work, anyway.
If it's only ever going to be supported on VS positions, then we could place it on the entry point itself (see struct EntryPoint
). Would you be interested to check and see how this works out in terms of code complexity?
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 checked the EntryPoint
way but I think the code would end up a lot more complex than it is now since the frontends would need to first read the invariant attribute, then look for all entry points that use that position builtin (since it can be in a struct) and set it on the entry point. Backends would also have trouble with this since they would have to map the invariant attribute back.
If the spec changes to allow the attribute to be used on locations, we can change the binding to be a struct with an inner field being the enum we have now and add the invariant flag on the struct.
I agree that it might not be the best spot for it however I tried to put it in multiple places and decided at last to put it here.
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.
That's a good insight, thank you!
Going to ask @jimblandy to take a look as well - on the API change only. |
Thanks, no hurry! |
d592d4c
to
b0443c1
Compare
Rebased to fix conflicts |
Jim seems to have missed this one, unfortunately. Let's go ahead and merge then. |
closes #1659
Implements the invariant attribute in all frontends and backends.
References
WGSL Invariant Attribute
SPIR-V Invariant Decoration (#18)
MSL Invariant Attribute
GLSL Invariant Qualifier
HLSL Variable Syntax
HLSL Function Declaration Syntax