[PATCH v3 0/5] MR283: vkd3d-shader/dxil: Implement DX instruction StoreOutput.
-- v3: vkd3d-shader/dxil: Implement DX instruction StoreOutput. vkd3d-shader/dxil: Declare shader outputs. vkd3d-shader/dxil: Implement the DXIL CALL instruction. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283
From: Conor McCarthy <cmccarthy(a)codeweavers.com> In DXIL these masks can have a left shift. --- libs/vkd3d-shader/spirv.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index d71f0a69..54c80a62 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4764,13 +4764,16 @@ static bool is_dual_source_blending(const struct spirv_compiler *compiler) static void calculate_clip_or_cull_distance_mask(const struct signature_element *e, uint32_t *mask) { + unsigned int write_mask; + if (e->semantic_index >= sizeof(*mask) * CHAR_BIT / VKD3D_VEC4_SIZE) { FIXME("Invalid semantic index %u for clip/cull distance.\n", e->semantic_index); return; } - *mask |= (e->mask & VKD3DSP_WRITEMASK_ALL) << (VKD3D_VEC4_SIZE * e->semantic_index); + write_mask = e->mask >> vkd3d_write_mask_get_component_idx(e->mask); + *mask |= (write_mask & VKD3DSP_WRITEMASK_ALL) << (VKD3D_VEC4_SIZE * e->semantic_index); } /* Emits arrayed SPIR-V built-in variables. */ -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Element masks can have a left shift in DXIL, but these must start at bit 0 in the register info. The SPIR-V declaration will either be a builtin or have SpvDecorationComponent. --- libs/vkd3d-shader/spirv.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 54c80a62..e0c8a92d 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4937,7 +4937,6 @@ static void spirv_compiler_emit_output(struct spirv_compiler *compiler, component_type = builtin->component_type; if (!builtin->spirv_array_size) output_component_count = builtin->component_count; - component_idx = 0; } else { @@ -4951,14 +4950,9 @@ static void spirv_compiler_emit_output(struct spirv_compiler *compiler, || needs_private_io_variable(builtin)) { use_private_variable = true; - reg_write_mask = write_mask; - } - else - { - component_idx = vkd3d_write_mask_get_component_idx(write_mask); - reg_write_mask = write_mask >> component_idx; } + reg_write_mask = write_mask >> component_idx; vkd3d_symbol_make_register(®_symbol, reg); if (rb_get(&compiler->symbol_table, ®_symbol)) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/dxil.c | 245 ++++++++++++++++++++++- libs/vkd3d-shader/ir.c | 2 +- libs/vkd3d-shader/spirv.c | 5 - libs/vkd3d-shader/vkd3d_shader_private.h | 8 + 4 files changed, 251 insertions(+), 9 deletions(-) diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index f9efe47f..e2aa41ce 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -22,6 +22,7 @@ #define VKD3D_SM6_VERSION_MINOR(version) (((version) >> 0) & 0xf) #define BITCODE_MAGIC VKD3D_MAKE_TAG('B', 'C', 0xc0, 0xde) +#define DXIL_OP_MAX_OPERANDS 17 enum bitcode_block_id { @@ -242,6 +243,8 @@ struct sm6_function struct sm6_block *blocks[1]; size_t block_count; + + size_t value_count; }; struct dxil_block @@ -293,6 +296,7 @@ struct sm6_parser struct sm6_value *values; size_t value_count; size_t value_capacity; + size_t cur_max_value; struct vkd3d_shader_parser p; }; @@ -1341,6 +1345,30 @@ static const struct sm6_type *sm6_type_get_pointer_to_type(const struct sm6_type return NULL; } +/* Never returns null for elem_idx 0. */ +static const struct sm6_type *sm6_type_get_scalar_type(const struct sm6_type *type, unsigned int elem_idx) +{ + switch (type->class) + { + case TYPE_CLASS_ARRAY: + case TYPE_CLASS_VECTOR: + if (elem_idx >= type->u.array.count) + return NULL; + return sm6_type_get_scalar_type(type->u.array.elem_type, 0); + + case TYPE_CLASS_POINTER: + return sm6_type_get_scalar_type(type->u.pointer.type, 0); + + case TYPE_CLASS_STRUCT: + if (elem_idx >= type->u.struc->elem_count) + return NULL; + return sm6_type_get_scalar_type(type->u.struc->elem_types[elem_idx], 0); + + default: + return type; + } +} + static const struct sm6_type *sm6_parser_get_type(struct sm6_parser *sm6, uint64_t type_id) { if (type_id >= sm6->type_count) @@ -1443,9 +1471,32 @@ static const char *sm6_parser_get_global_symbol_name(const struct sm6_parser *sm return NULL; } +static unsigned int register_get_uint_value(const struct vkd3d_shader_register *reg) +{ + if (!register_is_constant(reg)) + return UINT_MAX; + + if (reg->immconst_type == VKD3D_IMMCONST_VEC4) + WARN("Returning vec4.x.\n"); + + if (reg->type == VKD3DSPR_IMMCONST64) + { + if (reg->u.immconst_uint64[0] > UINT_MAX) + FIXME("Truncating 64-bit value.\n"); + return reg->u.immconst_uint64[0]; + } + + return reg->u.immconst_uint[0]; +} + +static inline bool sm6_value_is_function_dcl(const struct sm6_value *value) +{ + return value->value_type == VALUE_TYPE_FUNCTION; +} + static inline bool sm6_value_is_dx_intrinsic_dcl(const struct sm6_value *fn) { - assert(fn->value_type == VALUE_TYPE_FUNCTION); + assert(sm6_value_is_function_dcl(fn)); return fn->u.function.is_prototype && !strncmp(fn->u.function.name, "dx.op.", 6); } @@ -1455,6 +1506,16 @@ static inline struct sm6_value *sm6_parser_get_current_value(const struct sm6_pa return &sm6->values[sm6->value_count]; } +static inline bool sm6_value_is_register(const struct sm6_value *value) +{ + return value->value_type == VALUE_TYPE_REG; +} + +static inline bool sm6_value_is_constant(const struct sm6_value *value) +{ + return sm6_value_is_register(value) && register_is_constant(&value->u.reg); +} + static enum vkd3d_data_type vkd3d_data_type_from_sm6_type(const struct sm6_type *type) { if (type->class == TYPE_CLASS_INTEGER) @@ -1513,6 +1574,7 @@ static size_t sm6_parser_compute_max_value_count(struct sm6_parser *sm6, * overestimate the value count somewhat, but this should be no problem. */ value_count = size_add_with_overflow_check(value_count, max(block->record_count, 1u) - 1); sm6->value_capacity = max(sm6->value_capacity, value_count); + sm6->functions[sm6->function_count].value_count = value_count; /* The value count returns to its previous value after handling a function. */ if (value_count < SIZE_MAX) value_count = old_value_count; @@ -1524,6 +1586,72 @@ static size_t sm6_parser_compute_max_value_count(struct sm6_parser *sm6, return value_count; } +static size_t sm6_parser_get_value_index(const struct sm6_parser *sm6, uint64_t idx) +{ + size_t i; + + /* The value relative index is 32 bits. */ + if (idx > UINT32_MAX) + WARN("Ignoring upper 32 bits of relative index.\n"); + i = (uint32_t)sm6->value_count - (uint32_t)idx; + + /* This may underflow to produce a forward reference, but it must not exceeed the final value count. */ + if (i >= sm6->cur_max_value) + { + WARN("Invalid value index %"PRIx64" at %zu.\n", idx, sm6->value_count); + return SIZE_MAX; + } + if (i == sm6->value_count) + { + WARN("Invalid value self-reference at %zu.\n", sm6->value_count); + return SIZE_MAX; + } + + return i; +} + +static size_t sm6_parser_get_value_idx_by_ref(struct sm6_parser *sm6, const struct dxil_record *record, + const struct sm6_type *fwd_type, unsigned int *rec_idx) +{ + unsigned int idx; + uint64_t val_ref; + size_t operand; + + idx = *rec_idx; + if (!dxil_record_validate_operand_min_count(record, idx + 1, sm6)) + return SIZE_MAX; + val_ref = record->operands[idx++]; + + operand = sm6_parser_get_value_index(sm6, val_ref); + if (operand == SIZE_MAX) + return SIZE_MAX; + + if (operand >= sm6->value_count) + { + if (!fwd_type) + { + /* Forward references are followed by a type id unless an earlier operand set the type, + * or it is contained in a function declaration. */ + if (!dxil_record_validate_operand_min_count(record, idx + 1, sm6)) + return SIZE_MAX; + if (!(fwd_type = sm6_parser_get_type(sm6, record->operands[idx++]))) + return SIZE_MAX; + } + FIXME("Forward value references are not supported yet.\n"); + return SIZE_MAX; + } + *rec_idx = idx; + + return operand; +} + +static const struct sm6_value *sm6_parser_get_value_by_ref(struct sm6_parser *sm6, + const struct dxil_record *record, const struct sm6_type *type, unsigned int *rec_idx) +{ + size_t operand = sm6_parser_get_value_idx_by_ref(sm6, record, type, rec_idx); + return operand == SIZE_MAX ? NULL : &sm6->values[operand]; +} + static bool sm6_parser_declare_function(struct sm6_parser *sm6, const struct dxil_record *record) { const unsigned int max_count = 15; @@ -1838,6 +1966,100 @@ static struct sm6_block *sm6_block_create() return block; } +static void sm6_parser_emit_unhandled(struct sm6_parser *sm6, struct vkd3d_shader_instruction *ins, + struct sm6_value *dst) +{ + const struct sm6_type *type; + + ins->handler_idx = VKD3DSIH_NOP; + + if (!dst->type) + return; + + type = sm6_type_get_scalar_type(dst->type, 0); + shader_register_init(&dst->u.reg, VKD3DSPR_UNDEF, vkd3d_data_type_from_sm6_type(type), 0); + /* dst->is_undefined is not set here because it flags only explicitly undefined values. */ +} + +static void sm6_parser_decode_dx_op(struct sm6_parser *sm6, struct sm6_block *code_block, unsigned int op, + const char *name, const struct sm6_value **operands, unsigned int operand_count, + struct vkd3d_shader_instruction *ins, struct sm6_value *dst) +{ + FIXME("Unhandled dx intrinsic function id %u, '%s'.\n", op, name); + return sm6_parser_emit_unhandled(sm6, ins, dst); +} + +static void sm6_parser_emit_call(struct sm6_parser *sm6, const struct dxil_record *record, + struct sm6_block *code_block, struct vkd3d_shader_instruction *ins, struct sm6_value *dst) +{ + const struct sm6_value *operands[DXIL_OP_MAX_OPERANDS]; + const struct sm6_value *fn_value, *op_value; + unsigned int i = 1, j, operand_count; + const struct sm6_type *type = NULL; + uint64_t call_conv; + + if (!dxil_record_validate_operand_min_count(record, 3, sm6)) + return; + + /* TODO: load the 1-based attributes index from record->operands[0] and validate against attribute count. */ + + if ((call_conv = record->operands[i++]) & (1u << 15)) + type = sm6_parser_get_type(sm6, record->operands[i++]); + + if (!(fn_value = sm6_parser_get_value_by_ref(sm6, record, NULL, &i))) + return; + if (!sm6_value_is_function_dcl(fn_value)) + { + WARN("Target function value is not a function declaration.\n"); + return; + } + + if (!type) + { + type = fn_value->type->u.pointer.type; + } + else if (type != fn_value->type->u.pointer.type) + { + WARN("Explicit call type does not match function type.\n"); + type = fn_value->type->u.pointer.type; + } + + if (!sm6_type_is_void(type->u.function->ret_type)) + dst->type = type->u.function->ret_type; + + operand_count = type->u.function->param_count; + if (operand_count > ARRAY_SIZE(operands)) + { + FIXME("Unhandled operand count %u.\n", operand_count); + vkd3d_shader_parser_error(&sm6->p, VKD3D_SHADER_ERROR_DXIL_INVALID_OPERAND_COUNT, + "Unhandled operand count %u for dx intrinsic function.", operand_count); + return; + } + + for (j = 0; j < operand_count; ++j) + { + if (!(operands[j] = sm6_parser_get_value_by_ref(sm6, record, type->u.function->param_types[j], &i))) + return; + } + + if (!fn_value->u.function.is_prototype) + { + FIXME("Unhandled call to local function.\n"); + return; + } + if (!sm6_value_is_dx_intrinsic_dcl(fn_value)) + WARN("External function is not a dx intrinsic.\n"); + + op_value = operands[0]; + if (!sm6_value_is_constant(op_value) || !sm6_type_is_integer(op_value->type)) + { + WARN("dx intrinsic function id is not a constant int.\n"); + return; + } + sm6_parser_decode_dx_op(sm6, code_block, register_get_uint_value(&op_value->u.reg), + fn_value->u.function.name, &operands[1], operand_count - 1, ins, dst); +} + static void sm6_parser_emit_ret(struct sm6_parser *sm6, const struct dxil_record *record, struct sm6_block *code_block, struct vkd3d_shader_instruction *ins) { @@ -1856,11 +2078,12 @@ static enum vkd3d_result sm6_parser_function_init(struct sm6_parser *sm6, const struct vkd3d_shader_instruction *ins; const struct dxil_record *record; struct sm6_block *code_block; + bool ret_found, failed; struct sm6_value *dst; size_t i, block_idx; - bool ret_found; enum { + RESULT_VOID, RESULT_VALUE, RESULT_TERMINATE, } result_type; @@ -1907,7 +2130,9 @@ static enum vkd3d_result sm6_parser_function_init(struct sm6_parser *sm6, const } code_block = function->blocks[0]; - for (i = 1, block_idx = 0, ret_found = false; i < block->record_count; ++i) + sm6->cur_max_value = function->value_count; + + for (i = 1, block_idx = 0, ret_found = false, failed = false; i < block->record_count; ++i) { sm6->p.location.column = i; @@ -1931,6 +2156,10 @@ static enum vkd3d_result sm6_parser_function_init(struct sm6_parser *sm6, const record = block->records[i]; switch (record->code) { + case FUNC_CODE_INST_CALL: + sm6_parser_emit_call(sm6, record, code_block, ins, dst); + result_type = dst->type ? RESULT_VALUE : RESULT_VOID; + break; case FUNC_CODE_INST_RET: sm6_parser_emit_ret(sm6, record, code_block, ins); result_type = RESULT_TERMINATE; @@ -1946,12 +2175,20 @@ static enum vkd3d_result sm6_parser_function_init(struct sm6_parser *sm6, const ++block_idx; code_block = (block_idx < function->block_count) ? function->blocks[block_idx] : NULL; } + if (ins->handler_idx == VKD3DSIH_INVALID) + { + /* Parsing can continue if we have a result, otherwise the value references will be corrupted. */ + if (result_type == RESULT_VALUE && !dst->type) + return VKD3D_ERROR_INVALID_SHADER; + failed = true; + } if (code_block) code_block->instruction_count += ins->handler_idx != VKD3DSIH_NOP; else assert(ins->handler_idx == VKD3DSIH_NOP); sm6->value_count += !!dst->type; } + sm6->p.failed |= failed; if (!ret_found) { @@ -1996,6 +2233,8 @@ static enum vkd3d_result sm6_parser_module_init(struct sm6_parser *sm6, const st switch (block->id) { case CONSTANTS_BLOCK: + function = &sm6->functions[sm6->function_count]; + sm6->cur_max_value = function->value_count; return sm6_parser_constants_init(sm6, block); case FUNCTION_BLOCK: diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index d74f81af..628cbd79 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -230,7 +230,7 @@ static enum vkd3d_result flattener_flatten_phases(struct hull_flattener *normali return VKD3D_OK; } -static void shader_register_init(struct vkd3d_shader_register *reg, enum vkd3d_shader_register_type reg_type, +void shader_register_init(struct vkd3d_shader_register *reg, enum vkd3d_shader_register_type reg_type, enum vkd3d_data_type data_type, unsigned int idx_count) { reg->type = reg_type; diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index e0c8a92d..ae82b113 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -204,11 +204,6 @@ static inline bool register_is_undef(const struct vkd3d_shader_register *reg) return reg->type == VKD3DSPR_UNDEF; } -static inline bool register_is_constant(const struct vkd3d_shader_register *reg) -{ - return (reg->type == VKD3DSPR_IMMCONST || reg->type == VKD3DSPR_IMMCONST64); -} - static inline bool register_is_constant_or_undef(const struct vkd3d_shader_register *reg) { return register_is_constant(reg) || register_is_undef(reg); diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index d35f49a6..094231be 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -719,6 +719,9 @@ struct vkd3d_shader_register } u; }; +void shader_register_init(struct vkd3d_shader_register *reg, enum vkd3d_shader_register_type reg_type, + enum vkd3d_data_type data_type, unsigned int idx_count); + struct vkd3d_shader_dst_param { struct vkd3d_shader_register reg; @@ -999,6 +1002,11 @@ static inline bool vkd3d_shader_register_is_patch_constant(const struct vkd3d_sh return reg->type == VKD3DSPR_PATCHCONST; } +static inline bool register_is_constant(const struct vkd3d_shader_register *reg) +{ + return (reg->type == VKD3DSPR_IMMCONST || reg->type == VKD3DSPR_IMMCONST64); +} + struct vkd3d_shader_location { const char *source_name; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- include/vkd3d_shader.h | 2 + libs/vkd3d-shader/dxil.c | 96 +++++++++++++++++++++++++++ libs/vkd3d-shader/vkd3d_shader_main.c | 3 - 3 files changed, 98 insertions(+), 3 deletions(-) diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index 8133d240..9afb6919 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -1439,6 +1439,8 @@ enum vkd3d_shader_sysval_semantic VKD3D_SHADER_SV_TESS_FACTOR_TRIINT = 0x0e, VKD3D_SHADER_SV_TESS_FACTOR_LINEDET = 0x0f, VKD3D_SHADER_SV_TESS_FACTOR_LINEDEN = 0x10, + /** Render target; SV_Target in Direct3D shader model 6 shaders. */ + VKD3D_SHADER_SV_TARGET = 0x40, VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_SYSVAL_SEMANTIC), }; diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index e2aa41ce..72972fc0 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -290,6 +290,8 @@ struct sm6_parser struct sm6_symbol *global_symbols; size_t global_symbol_count; + struct vkd3d_shader_dst_param *output_params; + struct sm6_function *functions; size_t function_count; @@ -1944,6 +1946,81 @@ static enum vkd3d_result sm6_parser_globals_init(struct sm6_parser *sm6) return VKD3D_OK; } +static void dst_param_io_init(struct vkd3d_shader_dst_param *param, + const struct signature_element *e, enum vkd3d_shader_register_type reg_type) +{ + enum vkd3d_shader_component_type component_type; + + param->write_mask = e->mask; + param->modifiers = 0; + param->shift = 0; + /* DXIL types do not have signedness. Load signed elements as unsigned. */ + component_type = e->component_type == VKD3D_SHADER_COMPONENT_INT ? VKD3D_SHADER_COMPONENT_UINT : e->component_type; + shader_register_init(¶m->reg, reg_type, vkd3d_data_type_from_component_type(component_type), 0); +} + +static void sm6_parser_init_signature(struct sm6_parser *sm6, const struct shader_signature *s, + enum vkd3d_shader_register_type reg_type, struct vkd3d_shader_dst_param *params) +{ + struct vkd3d_shader_dst_param *param; + const struct signature_element *e; + unsigned int i; + + for (i = 0; i < s->element_count; ++i) + { + e = &s->elements[i]; + + param = ¶ms[i]; + dst_param_io_init(param, e, reg_type); + param->reg.idx[0].offset = i; + param->reg.idx_count = 1; + } +} + +static void sm6_parser_emit_signature(struct sm6_parser *sm6, const struct shader_signature *s, + enum vkd3d_shader_opcode handler_idx, enum vkd3d_shader_opcode siv_handler_idx, + struct vkd3d_shader_dst_param *params) +{ + struct vkd3d_shader_instruction *ins; + struct vkd3d_shader_dst_param *param; + const struct signature_element *e; + unsigned int i; + + for (i = 0; i < s->element_count; ++i) + { + e = &s->elements[i]; + + /* Do not check e->used_mask because in some cases it is zero for used elements. + * TODO: scan ahead for used I/O elements. */ + + if (e->sysval_semantic != VKD3D_SHADER_SV_NONE && e->sysval_semantic != VKD3D_SHADER_SV_TARGET) + { + ins = sm6_parser_add_instruction(sm6, siv_handler_idx); + param = &ins->declaration.register_semantic.reg; + ins->declaration.register_semantic.sysval_semantic = vkd3d_siv_from_sysval(e->sysval_semantic); + } + else + { + ins = sm6_parser_add_instruction(sm6, handler_idx); + param = &ins->declaration.dst; + } + + *param = params[i]; + } +} + +static void sm6_parser_init_output_signature(struct sm6_parser *sm6, const struct shader_signature *output_signature) +{ + sm6_parser_init_signature(sm6, output_signature, + (sm6->p.shader_version.type == VKD3D_SHADER_TYPE_PIXEL) ? VKD3DSPR_COLOROUT : VKD3DSPR_OUTPUT, + sm6->output_params); +} + +static void sm6_parser_emit_output_signature(struct sm6_parser *sm6, const struct shader_signature *output_signature) +{ + sm6_parser_emit_signature(sm6, output_signature, VKD3DSIH_DCL_OUTPUT, VKD3DSIH_DCL_OUTPUT_SIV, sm6->output_params); +} + static const struct sm6_value *sm6_parser_next_function_definition(struct sm6_parser *sm6) { size_t i, count = sm6->function_count; @@ -2342,6 +2419,7 @@ static const struct vkd3d_shader_parser_ops sm6_parser_ops = static enum vkd3d_result sm6_parser_init(struct sm6_parser *sm6, const uint32_t *byte_code, size_t byte_code_size, const char *source_name, struct vkd3d_shader_message_context *message_context) { + const struct shader_signature *output_signature = &sm6->p.shader_desc.output_signature; const struct vkd3d_shader_location location = {.source_name = source_name}; uint32_t version_token, dxil_version, token_count, magic; unsigned int chunk_offset, chunk_size; @@ -2497,6 +2575,14 @@ static enum vkd3d_result sm6_parser_init(struct sm6_parser *sm6, const uint32_t return ret; } + if (!(sm6->output_params = shader_parser_get_dst_params(&sm6->p, output_signature->element_count))) + { + ERR("Failed to allocate output parameters.\n"); + vkd3d_shader_error(message_context, &location, VKD3D_SHADER_ERROR_DXIL_OUT_OF_MEMORY, + "Out of memory allocating output parameters."); + return VKD3D_ERROR_OUT_OF_MEMORY; + } + function_count = dxil_block_compute_function_count(&sm6->root_block); if (!(sm6->functions = vkd3d_calloc(function_count, sizeof(*sm6->functions)))) { @@ -2527,6 +2613,8 @@ static enum vkd3d_result sm6_parser_init(struct sm6_parser *sm6, const uint32_t return ret; } + sm6_parser_init_output_signature(sm6, output_signature); + if ((ret = sm6_parser_module_init(sm6, &sm6->root_block, 0)) < 0) { if (ret == VKD3D_ERROR_OUT_OF_MEMORY) @@ -2540,6 +2628,14 @@ static enum vkd3d_result sm6_parser_init(struct sm6_parser *sm6, const uint32_t return ret; } + if (!sm6_parser_require_space(sm6, output_signature->element_count)) + { + vkd3d_shader_error(message_context, &location, VKD3D_SHADER_ERROR_DXIL_OUT_OF_MEMORY, + "Out of memory emitting shader signature declarations."); + return VKD3D_ERROR_OUT_OF_MEMORY; + } + sm6_parser_emit_output_signature(sm6, output_signature); + for (i = 0; i < sm6->function_count; ++i) { if (!sm6_block_emit_instructions(sm6->functions[i].blocks[0], sm6)) diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index e8259181..e95e47d4 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1748,9 +1748,6 @@ void *shader_param_allocator_get(struct vkd3d_shader_param_allocator *allocator, { void *params; - if (!count) - return NULL; - if (count > allocator->count - allocator->index) { struct vkd3d_shader_param_node *next = shader_param_allocator_node_create(allocator); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/dxil.c | 224 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 221 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index 72972fc0..ff4ad65c 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -139,6 +139,11 @@ enum bitcode_value_symtab_code VST_CODE_BBENTRY = 2, }; +enum dx_intrinsic_opcode +{ + DX_STORE_OUTPUT = 5, +}; + struct sm6_pointer_info { const struct sm6_type *type; @@ -1267,6 +1272,16 @@ static inline bool sm6_type_is_integer(const struct sm6_type *type) return type->class == TYPE_CLASS_INTEGER; } +static inline bool sm6_type_is_i8(const struct sm6_type *type) +{ + return type->class == TYPE_CLASS_INTEGER && type->u.width == 8; +} + +static inline bool sm6_type_is_i32(const struct sm6_type *type) +{ + return type->class == TYPE_CLASS_INTEGER && type->u.width == 32; +} + static inline bool sm6_type_is_floating_point(const struct sm6_type *type) { return type->class == TYPE_CLASS_FLOAT; @@ -1518,6 +1533,46 @@ static inline bool sm6_value_is_constant(const struct sm6_value *value) return sm6_value_is_register(value) && register_is_constant(&value->u.reg); } +static inline bool sm6_value_is_undef(const struct sm6_value *value) +{ + return sm6_value_is_register(value) && value->u.reg.type == VKD3DSPR_UNDEF; +} + +static inline unsigned int sm6_value_get_constant_uint(const struct sm6_value *value) +{ + if (!sm6_value_is_constant(value)) + return UINT_MAX; + return register_get_uint_value(&value->u.reg); +} + +static struct vkd3d_shader_src_param *instruction_src_params_alloc(struct vkd3d_shader_instruction *ins, + unsigned int count, struct sm6_parser *sm6) +{ + struct vkd3d_shader_src_param *params = shader_parser_get_src_params(&sm6->p, count); + if (!params) + { + sm6->p.failed = true; + return NULL; + } + ins->src = params; + ins->src_count = count; + return params; +} + +static struct vkd3d_shader_dst_param *instruction_dst_params_alloc(struct vkd3d_shader_instruction *ins, + unsigned int count, struct sm6_parser *sm6) +{ + struct vkd3d_shader_dst_param *params = shader_parser_get_dst_params(&sm6->p, count); + if (!params) + { + sm6->p.failed = true; + return NULL; + } + ins->dst = params; + ins->dst_count = count; + return params; +} + static enum vkd3d_data_type vkd3d_data_type_from_sm6_type(const struct sm6_type *type) { if (type->class == TYPE_CLASS_INTEGER) @@ -1551,6 +1606,47 @@ static enum vkd3d_data_type vkd3d_data_type_from_sm6_type(const struct sm6_type return VKD3D_DATA_UINT; } +static inline void dst_param_init_scalar(struct vkd3d_shader_dst_param *param, unsigned int component_idx) +{ + param->write_mask = 1u << component_idx; + param->modifiers = 0; + param->shift = 0; +} + +static inline void src_param_init(struct vkd3d_shader_src_param *param) +{ + param->swizzle = VKD3D_SHADER_SWIZZLE(X, X, X, X); + param->modifiers = VKD3DSPSM_NONE; +} + +static void src_param_init_from_value(struct vkd3d_shader_src_param *param, const struct sm6_value *src) +{ + src_param_init(param); + param->reg = src->u.reg; +} + +static void register_address_init(struct vkd3d_shader_register *reg, const struct sm6_value *address, + unsigned int idx, struct sm6_parser *sm6) +{ + assert(idx < ARRAY_SIZE(reg->idx)); + if (sm6_value_is_constant(address)) + { + reg->idx[idx].offset = sm6_value_get_constant_uint(address); + } + else if (sm6_value_is_undef(address)) + { + reg->idx[idx].offset = 0; + } + else + { + struct vkd3d_shader_src_param *rel_addr = shader_parser_get_src_params(&sm6->p, 1); + if (rel_addr) + src_param_init_from_value(rel_addr, address); + reg->idx[idx].offset = 0; + reg->idx[idx].rel_addr = rel_addr; + } +} + /* Recurse through the block tree while maintaining a current value count. The current * count is the sum of the global count plus all declarations within the current function. * Store into value_capacity the highest count seen. */ @@ -2043,6 +2139,117 @@ static struct sm6_block *sm6_block_create() return block; } +static void sm6_parser_emit_dx_store_output(struct sm6_parser *sm6, struct sm6_block *code_block, + enum dx_intrinsic_opcode op, const struct sm6_value **operands, struct vkd3d_shader_instruction *ins) +{ + struct vkd3d_shader_src_param *src_param; + struct vkd3d_shader_dst_param *dst_param; + const struct shader_signature *signature; + unsigned int element_id, column_index; + const struct signature_element *e; + const struct sm6_value *value; + + element_id = sm6_value_get_constant_uint(operands[0]); + column_index = sm6_value_get_constant_uint(operands[2]); + + signature = &sm6->p.shader_desc.output_signature; + if (element_id >= signature->element_count) + { + WARN("Invalid element id %u.\n", element_id); + return; + } + e = &signature->elements[element_id]; + + value = operands[3]; + if (!sm6_value_is_register(value)) + { + WARN("Source value is not a register.\n"); + return; + } + + shader_instruction_init(ins, VKD3DSIH_MOV); + + if (!(dst_param = instruction_dst_params_alloc(ins, 1, sm6))) + return; + dst_param_init_scalar(dst_param, column_index); + dst_param->reg = sm6->output_params[element_id].reg; + if (e->register_count > 1) + register_address_init(&dst_param->reg, operands[1], 0, sm6); + + if ((src_param = instruction_src_params_alloc(ins, 1, sm6))) + src_param_init_from_value(src_param, value); +} + +struct sm6_dx_opcode_info +{ + const char ret_type; + const char *operand_info; + void (*handler)(struct sm6_parser *, struct sm6_block *, enum dx_intrinsic_opcode, + const struct sm6_value **, struct vkd3d_shader_instruction *); +}; + +/* + 8 -> int8 + i -> int32 + v -> void + o -> overloaded + */ +static const struct sm6_dx_opcode_info sm6_dx_op_table[] = +{ + [DX_STORE_OUTPUT ] = {'v', "ii8o", sm6_parser_emit_dx_store_output}, +}; + +static bool sm6_parser_validate_operand_type(struct sm6_parser *sm6, const struct sm6_type *type, char info_type) +{ + switch (info_type) + { + case 0: + FIXME("Invalid operand count.\n"); + return false; + case '8': + return sm6_type_is_i8(type); + case 'i': + return sm6_type_is_i32(type); + case 'v': + return !type; + case 'o': + /* TODO: some type checking may be possible */ + return true; + default: + FIXME("Unhandled operand code '%c'.\n", info_type); + return false; + } +} + +static bool sm6_parser_validate_dx_op(struct sm6_parser *sm6, enum dx_intrinsic_opcode code, + const struct sm6_value **operands, unsigned int operand_count, struct sm6_value *dst) +{ + const struct sm6_dx_opcode_info *info; + unsigned int i; + bool ret; + + info = &sm6_dx_op_table[code]; + + if (!sm6_parser_validate_operand_type(sm6, dst->type, info->ret_type)) + { + WARN("Failed to validate return type for code %u.\n", code); + /* Return type validation failure is not so critical. We only need to set + * a data type for the SSA result. */ + } + + for (i = 0, ret = true; i < operand_count; ++i) + { + const struct sm6_value *op = operands[i]; + if (!sm6_value_is_register(op) || !sm6_parser_validate_operand_type(sm6, op->type, info->operand_info[i])) + { + WARN("Failed to validate operand %u for dx intrinsic code %u.\n", i + 1, code); + ret = false; + } + } + + return ret; +} + static void sm6_parser_emit_unhandled(struct sm6_parser *sm6, struct vkd3d_shader_instruction *ins, struct sm6_value *dst) { @@ -2058,12 +2265,20 @@ static void sm6_parser_emit_unhandled(struct sm6_parser *sm6, struct vkd3d_shade /* dst->is_undefined is not set here because it flags only explicitly undefined values. */ } -static void sm6_parser_decode_dx_op(struct sm6_parser *sm6, struct sm6_block *code_block, unsigned int op, +static void sm6_parser_decode_dx_op(struct sm6_parser *sm6, struct sm6_block *code_block, enum dx_intrinsic_opcode op, const char *name, const struct sm6_value **operands, unsigned int operand_count, struct vkd3d_shader_instruction *ins, struct sm6_value *dst) { - FIXME("Unhandled dx intrinsic function id %u, '%s'.\n", op, name); - return sm6_parser_emit_unhandled(sm6, ins, dst); + if (op >= ARRAY_SIZE(sm6_dx_op_table) || !sm6_dx_op_table[op].operand_info) + { + FIXME("Unhandled dx intrinsic function id %u, '%s'.\n", op, name); + return sm6_parser_emit_unhandled(sm6, ins, dst); + } + + if (sm6_parser_validate_dx_op(sm6, op, operands, operand_count, dst)) + sm6_dx_op_table[op].handler(sm6, code_block, op, operands, ins); + else + sm6_parser_emit_unhandled(sm6, ins, dst); } static void sm6_parser_emit_call(struct sm6_parser *sm6, const struct dxil_record *record, @@ -2246,6 +2461,9 @@ static enum vkd3d_result sm6_parser_function_init(struct sm6_parser *sm6, const FIXME("Unhandled dxil instruction %u.\n", record->code); return VKD3D_ERROR_INVALID_SHADER; } + /* Allocation failure. */ + if (sm6->p.failed) + break; if (result_type == RESULT_TERMINATE) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
return NULL; }
+static unsigned int register_get_uint_value(const struct vkd3d_shader_register *reg)
Would it make sense to check that the register has the right type here? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40441
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
+ enum vkd3d_shader_opcode handler_idx, enum vkd3d_shader_opcode siv_handler_idx, + struct vkd3d_shader_dst_param *params) +{ + struct vkd3d_shader_instruction *ins; + struct vkd3d_shader_dst_param *param; + const struct signature_element *e; + unsigned int i; + + for (i = 0; i < s->element_count; ++i) + { + e = &s->elements[i]; + + /* Do not check e->used_mask because in some cases it is zero for used elements. + * TODO: scan ahead for used I/O elements. */ + + if (e->sysval_semantic != VKD3D_SHADER_SV_NONE && e->sysval_semantic != VKD3D_SHADER_SV_TARGET) That's not really a question about the code, but rather about shaders: is there a specific reason why `SV_Target` is special here?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40442
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
+} + +static inline unsigned int sm6_value_get_constant_uint(const struct sm6_value *value) +{ + if (!sm6_value_is_constant(value)) + return UINT_MAX; + return register_get_uint_value(&value->u.reg); +} + +static struct vkd3d_shader_src_param *instruction_src_params_alloc(struct vkd3d_shader_instruction *ins, + unsigned int count, struct sm6_parser *sm6) +{ + struct vkd3d_shader_src_param *params = shader_parser_get_src_params(&sm6->p, count); + if (!params) + { + sm6->p.failed = true; Maybe an error message could be emitted at this point?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40443
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
code_block = (block_idx < function->block_count) ? function->blocks[block_idx] : NULL; } + if (ins->handler_idx == VKD3DSIH_INVALID) + { + /* Parsing can continue if we have a result, otherwise the value references will be corrupted. */ + if (result_type == RESULT_VALUE && !dst->type) + return VKD3D_ERROR_INVALID_SHADER; + failed = true; + } if (code_block) code_block->instruction_count += ins->handler_idx != VKD3DSIH_NOP; else assert(ins->handler_idx == VKD3DSIH_NOP); sm6->value_count += !!dst->type; } + sm6->p.failed |= failed;
But is `sm6->p.failed` ever checked again in some ancestor of this function? I can't find where. Why can't we directly return `VKD3D_RESULT_INVALID_SHADER`? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40444
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
++block_idx; code_block = (block_idx < function->block_count) ? function->blocks[block_idx] : NULL; } + if (ins->handler_idx == VKD3DSIH_INVALID) + { + /* Parsing can continue if we have a result, otherwise the value references will be corrupted. */ + if (result_type == RESULT_VALUE && !dst->type) + return VKD3D_ERROR_INVALID_SHADER;
Why not always returning the error? What is the advantage of doing some more parsing, if the only information we're going to return is just a generic failure error, without additional diagnostics? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40445
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
return block; }
+static void sm6_parser_emit_dx_store_output(struct sm6_parser *sm6, struct sm6_block *code_block, + enum dx_intrinsic_opcode op, const struct sm6_value **operands, struct vkd3d_shader_instruction *ins) +{ + struct vkd3d_shader_src_param *src_param; + struct vkd3d_shader_dst_param *dst_param; + const struct shader_signature *signature; + unsigned int element_id, column_index; + const struct signature_element *e; + const struct sm6_value *value; + + element_id = sm6_value_get_constant_uint(operands[0]); + column_index = sm6_value_get_constant_uint(operands[2]);
Why `column_index` rather than, say, `component_index`? Is there supposed to be a `row_index` somewhere? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40447
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
}
-static void sm6_parser_decode_dx_op(struct sm6_parser *sm6, struct sm6_block *code_block, unsigned int op, +static void sm6_parser_decode_dx_op(struct sm6_parser *sm6, struct sm6_block *code_block, enum dx_intrinsic_opcode op, const char *name, const struct sm6_value **operands, unsigned int operand_count, struct vkd3d_shader_instruction *ins, struct sm6_value *dst) { - FIXME("Unhandled dx intrinsic function id %u, '%s'.\n", op, name); - return sm6_parser_emit_unhandled(sm6, ins, dst); + if (op >= ARRAY_SIZE(sm6_dx_op_table) || !sm6_dx_op_table[op].operand_info) + { + FIXME("Unhandled dx intrinsic function id %u, '%s'.\n", op, name); + return sm6_parser_emit_unhandled(sm6, ins, dst); + } + + if (sm6_parser_validate_dx_op(sm6, op, operands, operand_count, dst)) So it seems that `name` is only used to check that the function is a DX intrinsic (if it starts with `"dx.op."`) but is otherwise ignored to determine which intrinsic to call (and the first operand is used instead). This sounds pretty funny, you confirm that's intended? Maybe there could be a little comment about that.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40446
It might be that this ship has already sailed, since this is probably something preceding this MR, but I feel a bit awkward about how errors are silently ignored in a number of code paths (see for example `sm6_parser_emit_dx_store_output()`, but it's not the only place). I understand that we don't want to be overly zealous with rejecting invalid shaders, because it might be that even if our translation isn't perfect the translated shader is still good enough so that, say, a certain game is playable. However, at least from the point of view of vkd3d-shader (which doesn't know how that shader is going to be used), this doesn't really feel correct. Maybe the average game doesn't really care if a shader is broken, since the bad case scenario is that the player just decides that the game isn't fun and proceeds doing something else. But other applications might be more demanding. For example, if you allow me a rather extreme example, a medical application might be much less failure-tolerant, to the point of preferring declaring a malfunction rather than plotting wrong medical data. I'm not aware of any such usage, currently, for vkd3d-shader, but I consider this an example to infer that the decision of still using a shader even if it's known to be incorrectly translated should not belong to vkd3d-shader. It makes sense for vkd3d-shader to still report an incorrectly translated shader as a best guess, but the error code should declare what's happening, so that the client can choose mea ningfully. What is the feeling about this issue? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40448
Failure of `sm6_value_get_constant_uint()` is not handled for `column_index`, and `sm6_parser_function_init()` returns `VKD3D_OK` on paramater allocation failure. I intend to fix these. Are there other issues? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40526
On Thu Jul 27 11:21:55 2023 +0000, Giovanni Mascellani wrote:
That's not really a question about the code, but rather about shaders: is there a specific reason why `SV_Target` is special here? In SM4/5, `SV_Target` is not declared as a sysval, so this is done to match.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40528
On Thu Jul 27 11:21:57 2023 +0000, Giovanni Mascellani wrote:
Why not always returning the error? What is the advantage of doing some more parsing, if the only information we're going to return is just a generic failure error, without additional diagnostics? TPF parsing continues after some failures, e.g. an unhandled instruction. I did this for consistency, but I have no objection to returning an error immediately for all failures. EDIT: Excluding unimplemented intrinsics, which return undefs so the shader can be used. This has proven useful.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40535
On Thu Jul 27 11:21:57 2023 +0000, Giovanni Mascellani wrote:
So it seems that `name` is only used to check that the function is a DX intrinsic (if it starts with `"dx.op."`) but is otherwise ignored to determine which intrinsic to call (and the first operand is used instead). This sounds pretty funny, you confirm that's intended? Maybe there could be a little comment about that. It's how Microsoft hacked shader instructions into LLVM. The first parameter is really an opcode.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40537
On Thu Jul 27 11:21:58 2023 +0000, Giovanni Mascellani wrote:
Why `column_index` rather than, say, `component_index`? Is there supposed to be a `row_index` somewhere? The DXIL docs use row and column terminology. Maybe `element_id` should be renamed `row_index`. `column_index` is not necessarily a component index. One row can have different declarations which can end up as separate SPIR-V variables.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40538
Why not always returning the error? What is the advantage of doing some more parsing, if the only information we're going to return is just a generic failure error, without additional diagnostics?
Well, we should be providing the additional diagnostics. But generally, for much the same reasons that e.g. the HLSL compiler doesn't abort at the first sign of trouble. There may be multiple unrelated issues in the same shader, and knowing about subsequent issues sooner rather than later is generally helpful. At the very least because it might give someone interested in working on an application a better sense of the amount of effort required to make it work.
It might be that this ship has already sailed, since this is probably something preceding this MR, but I feel a bit awkward about how errors are silently ignored in a number of code paths (see for example `sm6_parser_emit_dx_store_output()`, but it's not the only place). I understand that we don't want to be overly zealous with rejecting invalid shaders, because it might be that even if our translation isn't perfect the translated shader is still good enough so that, say, a certain game is playable.
No, I think you're right about that; the default behaviour should be to reject invalid shaders. We may want to be able to relax that in some cases, but we could introduce compile options for that, perhaps enabled by VKD3D_CONFIG flags on the vkd3d side. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/283#note_40578
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)