vkd3d-shader/spirv: Fixes for NonReadable decorators
The existing code reuses the same SPIR-V variable for all descriptors mapped to the same Vulkan binding, and applies the NonReadable decoration based on the VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_UAV_READ only. This potentially causes the decoration to be applied twice, should two non-read descriptors be mapped to the same variable, which isn't allowed in SPIR-V, and the validator complains.
And in the case two uav descriptors are mapped to the same variable, and one is read from while the other is not, the variable would get the NonReadable decorator, while being read from later.
The first case can be hit with the following minimal test, with VKD3D_SHADER_CONFIG=force_validation
:
(extracted and simplified from sm6-uav-rwtexture.shader_test)
[require]
shader model >= 6.0
[uav 0]
size (2d, 1, 1)
[uav 1]
size (2d, 1, 1)
[pixel shader]
RWTexture2D<float> u;
RWTexture2D<float> v;
float4 main() : sv_target
{
u[uint2(0, 0)] = 0.5;
v[uint2(0, 0)] = 0.5;
return 0;
}
[test]
draw quad
The second case can be hit with the same test, just modifying it to do v[uint2(0, 0)] = u[uint2(0, 0)];
, and although it doesn't explicit error, i believe it is incorrect to set the NonReadable decoration while still reading from the variable, which can be seen on the SPIR-V dump when tracing.
This fixes two test failures i hit on my machine with both radv and llvmpipe. (Those tests do not fail on CI due to debian's older version of spirv-tools.)
I am not sure if this fix is the best possible fix for this, so feedback is welcome.
Merge request reports
Activity
Do these issues potentially apply to other descriptor decorations as well? SpvDecorationCoherent from spirv_compiler_emit_resource_declaration() is the one I'd be most curious about.
I am not sure if this fix is the best possible fix for this, so feedback is welcome.
I think this largely makes sense, but if we're going to track "write_only" anyway, I think we may as well add the NonReadable decoration in spirv_compiler_build_descriptor_variable() itself. That may even allow us to avoid tracking "write_only" in struct vkd3d_descriptor_variable_info, particularly if we swap the order of the two commits in this series.
Looking at the descriptor handling code in the SPIR-V backend more broadly, I think we may be able to simplify/streamline some things by passing a struct vkd3d_shader_descriptor_info1 to spirv_compiler_build_descriptor_variable(). I think that's ultimately always called from spirv_compiler_emit_descriptor_declarations() where we trivially have that descriptor info, and would avoid the spirv_compiler_get_descriptor_info() call introduced by this series, as well as the existing one in spirv_compiler_emit_resource_declaration(). That's a bit of a larger change though, so perhaps best left as a follow-up for after the 1.13 release.
Do these issues potentially apply to other descriptor decorations as well?
yes, applies to every decoration, only
FuncParamAttr
andUserSemantic
are currently allowed to be duplicate, and even those can't be completely duplicate (their operands must be unique, but the latest spirv-tools doesn't check the operands, yet at least)--
i did notice that
spirv_compiler_emit_resource_declaration()
is only called fromspirv_compiler_emit_descriptor_declarations()
, so passing in the struct would make sense, and reduce the number of arguments for the former (since it passes in a lot of the fields from info1 as individual arguments)and this would allow us to trickle down the info1 struct to
spirv_compiler_build_descriptor_variable()
like you mentioned, as that extra call tospirv_compiler_get_descriptor_info()
was one thing i worried about (since it's an extra, basically unnecessary, linear search)--
i can work on that refactor now, or we improve this pr as is and i work on it after the 1.13 release.
Edited by navii can work on that refactor now, or we improve this pr as is and i work on it after the 1.13 release.
I think it makes the most sense to first improve this MR, and then do the refactor as a separate MR for after the release. (Which is planned for August 29, slightly over a week from now.)
mentioned in merge request !1024 (merged)