[PATCH 0/5] MR306: vkd3d-shader: Fix UAV coherence.
From: Conor McCarthy <cmccarthy(a)codeweavers.com> The flags start at bit 16 in the token, and bit 17 indicates an ROV. --- libs/vkd3d-shader/tpf.c | 4 ++-- libs/vkd3d-shader/vkd3d_shader_private.h | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 39c8b6f6..a41c1636 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -95,8 +95,8 @@ STATIC_ASSERT(SM4_MAX_SRC_COUNT <= SPIRV_MAX_SRC_COUNT); #define VKD3D_SM5_FP_ARRAY_SIZE_SHIFT 16 #define VKD3D_SM5_FP_TABLE_COUNT_MASK 0xffffu -#define VKD3D_SM5_UAV_FLAGS_SHIFT 15 -#define VKD3D_SM5_UAV_FLAGS_MASK (0x1ffu << VKD3D_SM5_UAV_FLAGS_SHIFT) +#define VKD3D_SM5_UAV_FLAGS_SHIFT 16 +#define VKD3D_SM5_UAV_FLAGS_MASK (0xffu << VKD3D_SM5_UAV_FLAGS_SHIFT) #define VKD3D_SM5_SYNC_FLAGS_SHIFT 11 #define VKD3D_SM5_SYNC_FLAGS_MASK (0xffu << VKD3D_SM5_SYNC_FLAGS_SHIFT) diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index c719085e..20f8cc63 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -618,8 +618,9 @@ enum vkd3d_shader_sync_flags enum vkd3d_shader_uav_flags { - VKD3DSUF_GLOBALLY_COHERENT = 0x2, - VKD3DSUF_ORDER_PRESERVING_COUNTER = 0x100, + VKD3DSUF_GLOBALLY_COHERENT = 0x1, + VKD3DSUF_RASTERISER_ORDERED_VIEW = 0x2, + VKD3DSUF_ORDER_PRESERVING_COUNTER = 0x80, }; enum vkd3d_tessellator_domain -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/306
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- include/vkd3d_shader.h | 3 +++ libs/vkd3d-shader/vkd3d_shader_main.c | 27 +++++++++++++++++++-------- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index e98aad4f..6926c750 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -1339,6 +1339,9 @@ enum vkd3d_shader_descriptor_info_flag /** The descriptor is a UAV resource, on which the shader performs * atomic ops. \since 1.6 */ VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_UAV_ATOMICS = 0x00000008, + /** The descriptor is a UAV resource, which is globally coherent. + * \since 1.9 */ + VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_UAV_GLOBALLY_COHERENT = 0x00000010, VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_DESCRIPTOR_INFO_FLAG), }; diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 3a2432c1..5b8d9c38 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -814,20 +814,29 @@ static void vkd3d_shader_scan_sampler_declaration(struct vkd3d_shader_scan_conte } static void vkd3d_shader_scan_resource_declaration(struct vkd3d_shader_scan_context *context, - const struct vkd3d_shader_resource *resource, enum vkd3d_shader_resource_type resource_type, - enum vkd3d_shader_resource_data_type resource_data_type) + const struct vkd3d_shader_instruction *instruction, const struct vkd3d_shader_resource *resource, + enum vkd3d_shader_resource_type resource_type, enum vkd3d_shader_resource_data_type resource_data_type) { enum vkd3d_shader_descriptor_type type; + unsigned int flags = 0; if (!context->scan_descriptor_info) return; if (resource->reg.reg.type == VKD3DSPR_UAV) + { + if (instruction->flags & VKD3DSUF_GLOBALLY_COHERENT) + flags = VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_UAV_GLOBALLY_COHERENT; type = VKD3D_SHADER_DESCRIPTOR_TYPE_UAV; + } else + { + if (instruction->flags) + FIXME("Unhandled flags %#x for SRV.\n", instruction->flags); type = VKD3D_SHADER_DESCRIPTOR_TYPE_SRV; + } vkd3d_shader_scan_add_descriptor(context, type, &resource->reg.reg, &resource->range, - resource_type, resource_data_type, 0); + resource_type, resource_data_type, flags); } static void vkd3d_shader_scan_typed_resource_declaration(struct vkd3d_shader_scan_context *context, @@ -885,7 +894,7 @@ static void vkd3d_shader_scan_typed_resource_declaration(struct vkd3d_shader_sca resource_data_type = VKD3D_SHADER_RESOURCE_DATA_FLOAT; } - vkd3d_shader_scan_resource_declaration(context, &semantic->resource, + vkd3d_shader_scan_resource_declaration(context, instruction, &semantic->resource, semantic->resource_type, resource_data_type); } @@ -919,13 +928,15 @@ static int vkd3d_shader_scan_instruction(struct vkd3d_shader_scan_context *conte break; case VKD3DSIH_DCL_RESOURCE_RAW: case VKD3DSIH_DCL_UAV_RAW: - vkd3d_shader_scan_resource_declaration(context, &instruction->declaration.raw_resource.resource, - VKD3D_SHADER_RESOURCE_BUFFER, VKD3D_SHADER_RESOURCE_DATA_UINT); + vkd3d_shader_scan_resource_declaration(context, instruction, + &instruction->declaration.raw_resource.resource, VKD3D_SHADER_RESOURCE_BUFFER, + VKD3D_SHADER_RESOURCE_DATA_UINT); break; case VKD3DSIH_DCL_RESOURCE_STRUCTURED: case VKD3DSIH_DCL_UAV_STRUCTURED: - vkd3d_shader_scan_resource_declaration(context, &instruction->declaration.structured_resource.resource, - VKD3D_SHADER_RESOURCE_BUFFER, VKD3D_SHADER_RESOURCE_DATA_UINT); + vkd3d_shader_scan_resource_declaration(context, instruction, + &instruction->declaration.structured_resource.resource, VKD3D_SHADER_RESOURCE_BUFFER, + VKD3D_SHADER_RESOURCE_DATA_UINT); break; case VKD3DSIH_IF: cf_info = vkd3d_shader_scan_push_cf_info(context); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/306
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index fa605f18..85911e37 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -5897,6 +5897,9 @@ static void spirv_compiler_emit_resource_declaration(struct spirv_compiler *comp if (!(d->flags & VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_UAV_READ)) vkd3d_spirv_build_op_decorate(builder, var_id, SpvDecorationNonReadable, NULL, 0); + if (d->flags & VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_UAV_GLOBALLY_COHERENT) + vkd3d_spirv_build_op_decorate(builder, var_id, SpvDecorationCoherent, NULL, 0); + if (d->flags & VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_UAV_COUNTER) { assert(structure_stride); /* counters are valid only for structured buffers */ @@ -5953,7 +5956,7 @@ static void spirv_compiler_emit_dcl_resource(struct spirv_compiler *compiler, uint32_t flags = instruction->flags; /* We don't distinguish between APPEND and COUNTER UAVs. */ - flags &= ~VKD3DSUF_ORDER_PRESERVING_COUNTER; + flags &= ~(VKD3DSUF_ORDER_PRESERVING_COUNTER | VKD3DSUF_GLOBALLY_COHERENT); if (flags) FIXME("Unhandled UAV flags %#x.\n", flags); @@ -5973,7 +5976,7 @@ static void spirv_compiler_emit_dcl_resource_raw(struct spirv_compiler *compiler uint32_t flags = instruction->flags; /* We don't distinguish between APPEND and COUNTER UAVs. */ - flags &= ~VKD3DSUF_ORDER_PRESERVING_COUNTER; + flags &= ~(VKD3DSUF_ORDER_PRESERVING_COUNTER | VKD3DSUF_GLOBALLY_COHERENT); if (flags) FIXME("Unhandled UAV flags %#x.\n", flags); @@ -5989,7 +5992,7 @@ static void spirv_compiler_emit_dcl_resource_structured(struct spirv_compiler *c uint32_t flags = instruction->flags; /* We don't distinguish between APPEND and COUNTER UAVs. */ - flags &= ~VKD3DSUF_ORDER_PRESERVING_COUNTER; + flags &= ~(VKD3DSUF_ORDER_PRESERVING_COUNTER | VKD3DSUF_GLOBALLY_COHERENT); if (flags) FIXME("Unhandled UAV flags %#x.\n", flags); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/306
From: Conor McCarthy <cmccarthy(a)codeweavers.com> The UniformMemory semantic applies the constraints to Uniform storage class memory, which matches how UAV variables are declared. --- libs/vkd3d-shader/spirv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 85911e37..f58f02da 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -9081,7 +9081,7 @@ static void spirv_compiler_emit_sync(struct spirv_compiler *compiler, if (flags & VKD3DSSF_GLOBAL_UAV) { memory_scope = SpvScopeDevice; - memory_semantics |= SpvMemorySemanticsImageMemoryMask; + memory_semantics |= SpvMemorySemanticsUniformMemoryMask | SpvMemorySemanticsImageMemoryMask; flags &= ~VKD3DSSF_GLOBAL_UAV; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/306
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 6 +++--- libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index f58f02da..26a53ea1 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -9078,11 +9078,11 @@ static void spirv_compiler_emit_sync(struct spirv_compiler *compiler, flags &= ~VKD3DSSF_THREAD_GROUP; } - if (flags & VKD3DSSF_GLOBAL_UAV) + if (flags & (VKD3DSSF_THREAD_GROUP_UAV | VKD3DSSF_GLOBAL_UAV)) { - memory_scope = SpvScopeDevice; + memory_scope = (flags & VKD3DSSF_GLOBAL_UAV) ? SpvScopeDevice : SpvScopeWorkgroup; memory_semantics |= SpvMemorySemanticsUniformMemoryMask | SpvMemorySemanticsImageMemoryMask; - flags &= ~VKD3DSSF_GLOBAL_UAV; + flags &= ~(VKD3DSSF_THREAD_GROUP_UAV | VKD3DSSF_GLOBAL_UAV); } if (flags) diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 20f8cc63..7a81bdbb 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -613,6 +613,7 @@ enum vkd3d_shader_sync_flags { VKD3DSSF_THREAD_GROUP = 0x1, VKD3DSSF_GROUP_SHARED_MEMORY = 0x2, + VKD3DSSF_THREAD_GROUP_UAV = 0x4, VKD3DSSF_GLOBAL_UAV = 0x8, }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/306
This conflicts with !304.
``` From b2800c80642369eddd7e65b52c3b7d4cb334ecee Mon Sep 17 00:00:00 2001 From: Conor McCarthy <cmccarthy(a)codeweavers.com> Date: Wed, 16 Aug 2023 13:31:49 +1000 Subject: [PATCH 1/5] vkd3d-shader/tpf: Fix extraction of the UAV declaration flags.
The flags start at bit 16 in the token, and bit 17 indicates an ROV. ```
So what's at bit 15 that breaks things? Also, technically this is making two separate changes. Should there be a corresponding change to shader_dump_uav_flags() as well?
```diff static void vkd3d_shader_scan_resource_declaration(struct vkd3d_shader_scan_context *context, - const struct vkd3d_shader_resource *resource, enum vkd3d_shader_resource_type resource_type, - enum vkd3d_shader_resource_data_type resource_data_type) + const struct vkd3d_shader_instruction *instruction, const struct vkd3d_shader_resource *resource, + enum vkd3d_shader_resource_type resource_type, enum vkd3d_shader_resource_data_type resource_data_type) { ```
Passing the entire instruction to vkd3d_shader_scan_resource_declaration() doesn't seem great to me; it seems to me that that means we're either passing too much information and could instead just pass e.g. the instruction flags, or that vkd3d_shader_scan_resource_declaration() isn't the right tool here, and the caller should instead call vkd3d_shader_scan_add_descriptor() itself.
```diff @@ -613,6 +613,7 @@ enum vkd3d_shader_sync_flags { VKD3DSSF_THREAD_GROUP = 0x1, VKD3DSSF_GROUP_SHARED_MEMORY = 0x2, + VKD3DSSF_THREAD_GROUP_UAV = 0x4, VKD3DSSF_GLOBAL_UAV = 0x8, }; ```
Like for the vkd3d_shader_uav_flags change, I'd expect a corresponding change to shader_dump_sync_flags() for this change. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/306#note_42862
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Henri Verbeet (＠hverbeet)