[PATCH v13 0/4] MR401: vkd3d-shader/dxil: Read CBV info.
-- v13: vkd3d-shader/dxil: Read CBV descriptors. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/401
From: Conor McCarthy <cmccarthy(a)codeweavers.com> DXIL constant buffer sizes are not aligned to 16 bytes. --- libs/vkd3d-shader/spirv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index bd9e7a33..2dab97cc 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -5639,7 +5639,8 @@ static void spirv_compiler_emit_cbv_declaration(struct spirv_compiler *compiler, reg.idx[1].offset = range->first; reg.idx[2].offset = range->last; - size = size_in_bytes / (VKD3D_VEC4_SIZE * sizeof(uint32_t)); + size = align(size_in_bytes, VKD3D_VEC4_SIZE * sizeof(uint32_t)); + size /= VKD3D_VEC4_SIZE * sizeof(uint32_t); if ((push_cb = spirv_compiler_find_push_constant_buffer(compiler, range))) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/401
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/dxil.c | 167 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 166 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index 4bfebea2..19ce2936 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -2821,6 +2821,11 @@ static bool sm6_metadata_value_is_node(const struct sm6_metadata_value *m) return m && m->type == VKD3D_METADATA_NODE; } +static bool sm6_metadata_value_is_value(const struct sm6_metadata_value *m) +{ + return m && m->type == VKD3D_METADATA_VALUE; +} + static bool sm6_metadata_value_is_string(const struct sm6_metadata_value *m) { return m && m->type == VKD3D_METADATA_STRING; @@ -3375,11 +3380,170 @@ static const struct sm6_metadata_value *sm6_parser_find_named_metadata(struct sm return NULL; } +static bool sm6_parser_resources_load_register_range(struct sm6_parser *sm6, + const struct sm6_metadata_node *node, struct vkd3d_shader_register_range *range) +{ + unsigned int size; + + if (!sm6_metadata_value_is_value(node->operands[1])) + { + WARN("Resource data type is not a value.\n"); + return false; + } + if (!sm6_type_is_pointer(node->operands[1]->value_type)) + { + WARN("Resource type is not a pointer.\n"); + vkd3d_shader_parser_warning(&sm6->p, VKD3D_SHADER_WARNING_DXIL_TYPE_MISMATCH, + "Resource metadata value type is not a pointer."); + } + + if (!sm6_metadata_get_uint_value(sm6, node->operands[3], &range->space)) + { + WARN("Failed to load register space.\n"); + return false; + } + if (!sm6_metadata_get_uint_value(sm6, node->operands[4], &range->first)) + { + WARN("Failed to load register first.\n"); + return false; + } + if (!sm6_metadata_get_uint_value(sm6, node->operands[5], &size)) + { + WARN("Failed to load register range size.\n"); + return false; + } + if (!size || (size != UINT_MAX && !vkd3d_bound_range(range->first, size, UINT_MAX))) + { + WARN("Invalid register range, first %u, size %u.\n", range->first, size); + return false; + } + range->last = (size == UINT_MAX) ? UINT_MAX : range->first + size - 1; + + return true; +} + +static enum vkd3d_result sm6_parser_resources_load_cbv(struct sm6_parser *sm6, + const struct sm6_metadata_node *node, const struct vkd3d_shader_register_range *range, + unsigned int register_id, struct vkd3d_shader_instruction *ins) +{ + struct vkd3d_shader_register *reg; + unsigned int buffer_size; + + if (node->operand_count < 7) + { + WARN("Invalid operand count %u.\n", node->operand_count); + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_OPERAND_COUNT, + "Invalid operand count %u for a CBV descriptor.", node->operand_count); + return VKD3D_ERROR_INVALID_SHADER; + } + if (node->operand_count > 7 && node->operands[7]) + { + WARN("Ignoring %u extra operands.\n", node->operand_count - 7); + vkd3d_shader_parser_warning(&sm6->p, VKD3D_SHADER_WARNING_DXIL_IGNORING_OPERANDS, + "Ignoring %u extra operands for a CBV descriptor.", node->operand_count - 7); + } + + if (!sm6_metadata_get_uint_value(sm6, node->operands[6], &buffer_size)) + { + WARN("Failed to load buffer size.\n"); + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_RESOURCES, + "Constant buffer size metadata value is not an integer."); + return VKD3D_ERROR_INVALID_SHADER; + } + + vsir_instruction_init(ins, &sm6->p.location, VKD3DSIH_DCL_CONSTANT_BUFFER); + ins->resource_type = VKD3D_SHADER_RESOURCE_BUFFER; + ins->declaration.cb.size = buffer_size; + ins->declaration.cb.src.swizzle = VKD3D_SHADER_NO_SWIZZLE; + ins->declaration.cb.src.modifiers = VKD3DSPSM_NONE; + + reg = &ins->declaration.cb.src.reg; + vsir_register_init(reg, VKD3DSPR_CONSTBUFFER, VKD3D_DATA_FLOAT, 3); + reg->idx[0].offset = register_id; + reg->idx[1].offset = range->first; + reg->idx[2].offset = range->last; + + ins->declaration.cb.range = *range; + + return VKD3D_OK; +} + +static enum vkd3d_result sm6_parser_descriptor_type_init(struct sm6_parser *sm6, + enum vkd3d_shader_descriptor_type type, const struct sm6_metadata_node *descriptor_node) +{ + struct vkd3d_shader_register_range range; + struct vkd3d_shader_instruction *ins; + const struct sm6_metadata_node *node; + const struct sm6_metadata_value *m; + unsigned int i, register_id; + enum vkd3d_result ret; + + for (i = 0; i < descriptor_node->operand_count; ++i) + { + m = descriptor_node->operands[i]; + if (!sm6_metadata_value_is_node(m)) + { + WARN("Resource descriptor is not a node.\n"); + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_RESOURCES, + "Resource descriptor is not a metadata node."); + return VKD3D_ERROR_INVALID_SHADER; + } + + node = m->u.node; + if (node->operand_count < 6) + { + WARN("Invalid operand count %u.\n", node->operand_count); + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_OPERAND_COUNT, + "Invalid operand count %u for a descriptor.", node->operand_count); + return VKD3D_ERROR_INVALID_SHADER; + } + + if (!sm6_metadata_get_uint_value(sm6, node->operands[0], ®ister_id)) + { + WARN("Failed to load resource id.\n"); + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_RESOURCES, + "Resource id metadata value is not an integer."); + return VKD3D_ERROR_INVALID_SHADER; + } + + if (!sm6_parser_resources_load_register_range(sm6, node, &range)) + { + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_RESOURCES, + "Resource register range is invalid."); + return VKD3D_ERROR_INVALID_SHADER; + } + + if (!(ins = sm6_parser_require_space(sm6, 1))) + { + ERR("Failed to allocate instruction.\n"); + return VKD3D_ERROR_OUT_OF_MEMORY; + } + + switch (type) + { + case VKD3D_SHADER_DESCRIPTOR_TYPE_CBV: + if ((ret = sm6_parser_resources_load_cbv(sm6, node, &range, register_id, ins)) < 0) + return ret; + break; + default: + FIXME("Unsupported descriptor type %u.\n", type); + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_RESOURCES, + "Resource descriptor type %u is unsupported.", type); + return VKD3D_ERROR_INVALID_SHADER; + } + + ++sm6->p.instructions.count; + } + + return VKD3D_OK; +} + static enum vkd3d_result sm6_parser_resources_init(struct sm6_parser *sm6) { const struct sm6_metadata_value *m = sm6_parser_find_named_metadata(sm6, "dx.resources"); enum vkd3d_shader_descriptor_type type; const struct sm6_metadata_node *node; + enum vkd3d_result ret; if (!m) return VKD3D_OK; @@ -3406,7 +3570,8 @@ static enum vkd3d_result sm6_parser_resources_init(struct sm6_parser *sm6) return VKD3D_ERROR_INVALID_SHADER; } - FIXME("Descriptor info is unhandled.\n"); + if ((ret = sm6_parser_descriptor_type_init(sm6, type, m->u.node)) < 0) + return ret; } return VKD3D_OK; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/401
From: Conor McCarthy <cmccarthy(a)codeweavers.com> DXIL declares CBV sizes in bytes and they are not aligned to 16 bytes. --- libs/vkd3d-shader/d3d_asm.c | 4 +++- libs/vkd3d-shader/tpf.c | 2 ++ libs/vkd3d-shader/vkd3d_shader_main.c | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index 5a75cf25..d97c9c82 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -1621,8 +1621,10 @@ static void shader_dump_instruction(struct vkd3d_d3d_asm_compiler *compiler, case VKD3DSIH_DCL_CONSTANT_BUFFER: vkd3d_string_buffer_printf(buffer, " "); shader_dump_register(compiler, &ins->declaration.cb.src.reg, true); - if (shader_ver_ge(&compiler->shader_version, 5, 1)) + if (shader_ver_ge(&compiler->shader_version, 6, 0)) shader_print_subscript(compiler, ins->declaration.cb.size, NULL); + else if (shader_ver_ge(&compiler->shader_version, 5, 1)) + shader_print_subscript(compiler, ins->declaration.cb.size / VKD3D_VEC4_SIZE / sizeof(float), NULL); shader_addline(buffer, ", %s", ins->flags & VKD3DSI_INDEXED_DYNAMIC ? "dynamicIndexed" : "immediateIndexed"); shader_dump_register_space(compiler, ins->declaration.cb.range.space); diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index bf4c03c0..fd75d73e 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -884,6 +884,8 @@ static void shader_sm4_read_dcl_constant_buffer(struct vkd3d_shader_instruction ins->declaration.cb.size = *tokens++; shader_sm4_read_register_space(priv, &tokens, end, &ins->declaration.cb.range.space); } + + ins->declaration.cb.size *= VKD3D_VEC4_SIZE * sizeof(float); } static void shader_sm4_read_dcl_sampler(struct vkd3d_shader_instruction *ins, uint32_t opcode, uint32_t opcode_token, diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 052edeb5..7aaec49f 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -817,7 +817,7 @@ static void vkd3d_shader_scan_constant_buffer_declaration(struct vkd3d_shader_sc if (!(d = vkd3d_shader_scan_add_descriptor(context, VKD3D_SHADER_DESCRIPTOR_TYPE_CBV, &cb->src.reg, &cb->range, VKD3D_SHADER_RESOURCE_BUFFER, VKD3D_SHADER_RESOURCE_DATA_UINT))) return; - d->buffer_size = cb->size * 16; + d->buffer_size = cb->size; } static void vkd3d_shader_scan_sampler_declaration(struct vkd3d_shader_scan_context *context, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/401
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/dxil.c | 41 ++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 42 insertions(+) diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index 06faddac..4bfebea2 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -25,6 +25,7 @@ #define BITCODE_MAGIC VKD3D_MAKE_TAG('B', 'C', 0xc0, 0xde) #define DXIL_OP_MAX_OPERANDS 17 +static const unsigned int SHADER_DESCRIPTOR_TYPE_COUNT = 4; static const unsigned int dx_max_thread_group_size[3] = {1024, 1024, 64}; @@ -3374,6 +3375,43 @@ static const struct sm6_metadata_value *sm6_parser_find_named_metadata(struct sm return NULL; } +static enum vkd3d_result sm6_parser_resources_init(struct sm6_parser *sm6) +{ + const struct sm6_metadata_value *m = sm6_parser_find_named_metadata(sm6, "dx.resources"); + enum vkd3d_shader_descriptor_type type; + const struct sm6_metadata_node *node; + + if (!m) + return VKD3D_OK; + + node = m->u.node; + if (node->operand_count != SHADER_DESCRIPTOR_TYPE_COUNT) + { + WARN("Unexpected descriptor type count %u.\n", node->operand_count); + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_RESOURCES, + "Descriptor type count %u is invalid.", node->operand_count); + return VKD3D_ERROR_INVALID_SHADER; + } + + for (type = 0; type < SHADER_DESCRIPTOR_TYPE_COUNT; ++type) + { + if (!(m = node->operands[type])) + continue; + + if (!sm6_metadata_value_is_node(m)) + { + WARN("Resource list is not a node.\n"); + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_RESOURCES, + "Resource list is not a metadata node."); + return VKD3D_ERROR_INVALID_SHADER; + } + + FIXME("Descriptor info is unhandled.\n"); + } + + return VKD3D_OK; +} + static void signature_element_read_additional_element_values(struct signature_element *e, const struct sm6_metadata_node *node, struct sm6_parser *sm6) { @@ -4176,6 +4214,9 @@ static enum vkd3d_result sm6_parser_init(struct sm6_parser *sm6, const uint32_t if ((ret = sm6_parser_entry_point_init(sm6)) < 0) return ret; + if ((ret = sm6_parser_resources_init(sm6)) < 0) + return ret; + if ((ret = sm6_parser_module_init(sm6, &sm6->root_block, 0)) < 0) { if (ret == VKD3D_ERROR_OUT_OF_MEMORY) diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 85041f2a..cd5cc593 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -180,6 +180,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_DXIL_INVALID_ENTRY_POINT = 8015, VKD3D_SHADER_ERROR_DXIL_INVALID_SIGNATURE = 8016, VKD3D_SHADER_ERROR_DXIL_INVALID_PROPERTIES = 8017, + VKD3D_SHADER_ERROR_DXIL_INVALID_RESOURCES = 8018, VKD3D_SHADER_WARNING_DXIL_UNKNOWN_MAGIC_NUMBER = 8300, VKD3D_SHADER_WARNING_DXIL_UNKNOWN_SHADER_TYPE = 8301, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/401
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/401
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/d3d_asm.c:
case VKD3DSIH_DCL_CONSTANT_BUFFER: vkd3d_string_buffer_printf(buffer, " "); shader_dump_register(compiler, &ins->declaration.cb.src.reg, true); - if (shader_ver_ge(&compiler->shader_version, 5, 1)) + if (shader_ver_ge(&compiler->shader_version, 6, 0)) shader_print_subscript(compiler, ins->declaration.cb.size, NULL); + else if (shader_ver_ge(&compiler->shader_version, 5, 1)) + shader_print_subscript(compiler, ins->declaration.cb.size / VKD3D_VEC4_SIZE / sizeof(float), NULL);
I don't oppose to this specific change, but it feels like another sign of the fact that using VSIR both for disassembling and as an internal IR is going to be more and more complicated. In particular, with this change if you look at a `dcl_constant_buffer` instruction you don't know what's the actual size unless you know which SM you're using, which I don't find a very desirable property. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/401#note_49285
I don't oppose to this specific change, but it feels like another sign of the fact that using VSIR both for disassembling and as an internal IR is going to be more and more complicated. In particular, with this change if you look at a `dcl_constant_buffer` instruction you don't know what's the actual size unless you know which SM you're using, which I don't find a very desirable property.
I agree with the general sentiment, but note that there's no inconsistency on the vsir level; this is a change purely to the output format. And while shader model 6 d3d-asm is a format invented by us, there are certainly shader model specific d3d-asm changes that are out of our control, like those between SM3 and SM4, or those between SM4 and SM5.1. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/401#note_49287
I had much the same thought when I made this change. A similar problem exists with the CBufferLoad instruction (vs the more common CBufferLoadLegacy), which has no specific alignment restriction and specifies the offset in bytes. We don't really need that for a while though. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/401#note_49296
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)