-
Notifications
You must be signed in to change notification settings - Fork 560
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
OpAccessChain swizzling itself is not handled correctly when input type is widened #2287
Comments
In that case, it was supposed to insert a Actually, in this case, since we're already chaining into the vector, we could probably avoid inserting the swizzle entirely. |
This seems more like a case of the type hierarchy for padded vectors being completely wrong, causing the code to think that the expressions vecsize is larger than what it's supposed to be, which causes it to insert the wrong swizzle. |
I'll test the changes you made in a second locally. Thanks for fixing! |
Yup, this fixed all related failures of maintenace4 related CTS tests. Thanks again! |
I found this during investigation of Vulkan CTS failures related KhronosGroup/MoltenVK#2116, but I am too inexperienced with this codebase to properly assess where it's going wrong and what should be changed.
shader.zip
Essentially, when compiling the attached SPIR-V to MSL using
--msl-shader-input 0 any32 4
as a command line option, the following MSL is generated:And the Metal compiler returns these errors:
After looking through the SPIRV-Cross source, I found these lines being the culprit as they add the additional
.x
and.y
swizzles:SPIRV-Cross/spirv_msl.cpp
Lines 8390 to 8406 in 5f7a6de
Here, the code is intended to add swizzles when the vector width got widened (for example through shader inputs) to correctly access only the relevant components that the original code intended to access. However, this doesn't work in this case, as the vector width is increased from 2 to 4, and the original SPIR-V assembly already swizzled the original vector (the
%uint_0
and%uint_1
operands essentially do the.x
and.y
respectively):The
expr_type
initially contains a type with avecsize
of 4, and through the parent_type loop thing it get's changed to a type with avecsize
of 2 (I guess this is the original type).I see two solutions for this, but I don't know how well they'd integrate with the codebase and other cases I might not be considering. Perhaps @cdavis5e can chip in here, as you wrote the original code I linked.
(1) The
access_chain_internal
function should somehow return which type the last access in the chain has, which in this case would simply be afloat
. Not sure how well this would work with the original use case for that block.(2) There's perhaps some issue with the vector extension, and the
expr_type
should actually be a type with avecsize
of 1 and there's some bug somewhere else I don't know about. Or that the swizzling should only happen when we know the type was actually modified.The text was updated successfully, but these errors were encountered: