Skip to content
Snippets Groups Projects

vkd3d-shader/spirv: Fixes for NonReadable decorators

Merged navi requested to merge navi/vkd3d:non-readable-descriptor into master

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.

Edited by navi

Merge request reports

Merge request pipeline #30525 skipped

Merge request pipeline skipped for 94c74d2c

Approval is optional

Merged by Henri VerbeetHenri Verbeet 7 months ago (Aug 20, 2024 7:33pm UTC)

Merge details

  • Changes merged into with 94c74d2c.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • navi changed the description

    changed the description

  • 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.

  • Author Contributor

    Do these issues potentially apply to other descriptor decorations as well?

    yes, applies to every decoration, only FuncParamAttr and UserSemantic 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 from spirv_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 to spirv_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 navi
  • i 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.)

  • navi added 3 commits

    added 3 commits

    • 355d0d92 - vkd3d-shader/spirv: Avoid decorating variable with NonReadable multiple times.
    • 63e27e98 - vkd3d-shader/spirv: Use unique SPIR-V variables for descriptors where visibility differs.
    • 540e4de9 - vkd3d-shader/spirv: Avoid decorating variable with Coherent multiple times.

    Compare with previous version

  • Author Contributor

    I've moved the decoration assignment into spirv_compiler_build_descriptor_variable() for both NonReadable and Coherent (should i rebase both commits into one?)

    we no longer track write_only in vkd3d_descriptor_variable_info.

  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 3 commits

    added 3 commits

    • 343c7942 - vkd3d-shader/spirv: Avoid decorating variables multiple times with NonReadable.
    • a47c23fa - vkd3d-shader/spirv: Use unique SPIR-V variables for descriptors where visibility differs.
    • 94c74d2c - vkd3d-shader/spirv: Avoid decorating variables multiple times with Coherent.

    Compare with previous version

  • navi mentioned in merge request !1024 (merged)

    mentioned in merge request !1024 (merged)

Please register or sign in to reply
Loading