-
Notifications
You must be signed in to change notification settings - Fork 195
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
[glsl-out] Handle vector bitcasts #1966
[glsl-out] Handle vector bitcasts #1966
Conversation
I don't think this is the right approach, I think the problem is the backend generating the bad casts not the frontend generating the bad IR because it sets the width, but I need either @kvark or @jimblandy to confirm |
SPIR-V So I think @JCapucho is right here: if the back end is looking at the absence of a bit width and generating an Here's the code in typifier.rs that says what the output of an
Every path chooses the resulting |
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.
This needs some test coverage. Snapshot tests that include a generated use of each of the GLSL functions in the output via this code should suffice, and that'll get the validator to check that it's valid.
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 very much for the tests - they look great.
I'm still concerned about code duplication. Could we go around one more time on this?
src/back/glsl/mod.rs
Outdated
(Sk::Float, Sk::Uint) => "floatBitsToUint", | ||
(Sk::Sint, Sk::Float) => "intBitsToFloat", | ||
(Sk::Uint, Sk::Float) => "uintBitsToFloat", | ||
let conv_op = match (source_kind, target_kind, vector_size) { |
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.
It seems like almost all of these match arms could be handled with a single default call to self.write_value_type(inner)
. I'd like to see if that works before we duplicate all those variations 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.
Oh nice, that makes things super easy!
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.
Looks good - thank you!
This fixes an issue where an
OpBitcast
on a vector didn't generate the correct glsl: