[PATCH v13 0/7] MR450: vkd3d-shader/ir: Flatten structured control flow instructions.
-- v13: vkd3d-shader/ir: Store code block names in struct vkd3d_shader_desc. vkd3d-shader/ir: Flatten SWITCH/CASE/DEFAULT/ENDSWITCH control flow instructions. vkd3d-shader/ir: Flatten LOOP/BREAK/CONTINUE/ENDLOOP control flow instructions. vkd3d-shader/ir: Flatten IF/ELSE/ENDIF control flow instructions. vkd3d-shader/spirv: Handle RETP in spirv_compiler_handle_instruction(). vkd3d-shader/spirv: Handle DISCARD and TEXKILL in spirv_compiler_handle_instruction(). https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Ensures they are loaded only once per function independent of the control flow graph. --- libs/vkd3d-shader/spirv.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index dc3d96313..4d159430a 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2411,6 +2411,7 @@ static bool is_in_fork_or_join_phase(const struct spirv_compiler *compiler) } static void spirv_compiler_emit_initial_declarations(struct spirv_compiler *compiler); +static size_t spirv_compiler_get_current_function_location(struct spirv_compiler *compiler); static const char *spirv_compiler_get_entry_point_name(const struct spirv_compiler *compiler) { @@ -3478,11 +3479,13 @@ static uint32_t spirv_compiler_get_descriptor_index(struct spirv_compiler *compi index_ids[0] = compiler->descriptor_offsets_member_id; index_ids[1] = spirv_compiler_get_constant_uint(compiler, push_constant_index); ptr_type_id = vkd3d_spirv_get_op_type_pointer(builder, SpvStorageClassPushConstant, type_id); + vkd3d_spirv_begin_function_stream_insertion(builder, + spirv_compiler_get_current_function_location(compiler)); ptr_id = vkd3d_spirv_build_op_in_bounds_access_chain(builder, ptr_type_id, compiler->push_constants_var_id, index_ids, 2); offset_id = vkd3d_spirv_build_op_load(builder, type_id, ptr_id, SpvMemoryAccessMaskNone); - if (!compiler->control_flow_depth) - compiler->descriptor_offset_ids[push_constant_index] = offset_id; + vkd3d_spirv_end_function_stream_insertion(builder); + compiler->descriptor_offset_ids[push_constant_index] = offset_id; } index_id = vkd3d_spirv_build_op_iadd(builder, type_id, index_id, offset_id); } @@ -6612,6 +6615,9 @@ static void spirv_compiler_emit_hull_shader_main(struct spirv_compiler *compiler struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; uint32_t void_id; + /* If a patch constant function used descriptor indexing the offsets must be reloaded. */ + memset(compiler->descriptor_offset_ids, 0, compiler->offset_info.descriptor_table_count + * sizeof(*compiler->descriptor_offset_ids)); vkd3d_spirv_builder_begin_main_function(builder); void_id = vkd3d_spirv_get_op_type_void(builder); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 4d159430a..00960c897 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -7904,11 +7904,6 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c spirv_compiler_emit_retc(compiler, instruction); break; - case VKD3DSIH_DISCARD: - case VKD3DSIH_TEXKILL: - spirv_compiler_emit_kill(compiler, instruction); - break; - default: ERR("Unexpected instruction %#x.\n", instruction->handler_idx); break; @@ -9676,7 +9671,6 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_CONTINUE: case VKD3DSIH_CONTINUEP: case VKD3DSIH_DEFAULT: - case VKD3DSIH_DISCARD: case VKD3DSIH_ELSE: case VKD3DSIH_ENDIF: case VKD3DSIH_ENDLOOP: @@ -9686,9 +9680,12 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_RET: case VKD3DSIH_RETP: case VKD3DSIH_SWITCH: - case VKD3DSIH_TEXKILL: ret = spirv_compiler_emit_control_flow_instruction(compiler, instruction); break; + case VKD3DSIH_DISCARD: + case VKD3DSIH_TEXKILL: + spirv_compiler_emit_kill(compiler, instruction); + break; case VKD3DSIH_DSX: case VKD3DSIH_DSX_COARSE: case VKD3DSIH_DSX_FINE: -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 00960c897..dc295d8ac 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -7900,10 +7900,6 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c compiler->main_block_open = false; break; - case VKD3DSIH_RETP: - spirv_compiler_emit_retc(compiler, instruction); - break; - default: ERR("Unexpected instruction %#x.\n", instruction->handler_idx); break; @@ -9678,10 +9674,12 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_IF: case VKD3DSIH_LOOP: case VKD3DSIH_RET: - case VKD3DSIH_RETP: case VKD3DSIH_SWITCH: ret = spirv_compiler_emit_control_flow_instruction(compiler, instruction); break; + case VKD3DSIH_RETP: + spirv_compiler_emit_retc(compiler, instruction); + break; case VKD3DSIH_DISCARD: case VKD3DSIH_TEXKILL: spirv_compiler_emit_kill(compiler, instruction); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/d3d_asm.c | 44 ++- libs/vkd3d-shader/ir.c | 327 +++++++++++++++++++++++ libs/vkd3d-shader/spirv.c | 180 +++++++------ libs/vkd3d-shader/vkd3d_shader_private.h | 2 + 4 files changed, 461 insertions(+), 92 deletions(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index 82d1d71d9..0b13b8c0a 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -44,6 +44,7 @@ static const char * const shader_opcode_names[] = [VKD3DSIH_BEM ] = "bem", [VKD3DSIH_BFI ] = "bfi", [VKD3DSIH_BFREV ] = "bfrev", + [VKD3DSIH_BRANCH ] = "branch", [VKD3DSIH_BREAK ] = "break", [VKD3DSIH_BREAKC ] = "breakc", [VKD3DSIH_BREAKP ] = "breakp", @@ -828,6 +829,13 @@ static void shader_print_bool_literal(struct vkd3d_d3d_asm_compiler *compiler, compiler->colours.literal, b ? "true" : "false", compiler->colours.reset, suffix); } +static void shader_print_label_id(struct vkd3d_d3d_asm_compiler *compiler, + const char *prefix, unsigned int label_id) +{ + vkd3d_string_buffer_printf(&compiler->buffer, "%s%sl%u%s", prefix, + compiler->colours.reg, label_id, compiler->colours.reset); +} + static void shader_print_subscript(struct vkd3d_d3d_asm_compiler *compiler, unsigned int offset, const struct vkd3d_shader_src_param *rel_addr) { @@ -860,6 +868,12 @@ static void shader_dump_register(struct vkd3d_d3d_asm_compiler *compiler, const static const char * const rastout_reg_names[] = {"oPos", "oFog", "oPts"}; static const char * const misctype_reg_names[] = {"vPos", "vFace"}; + if (reg->type == VKD3DSPR_LABEL) + { + shader_print_label_id(compiler, "", reg->idx[0].offset); + return; + } + shader_addline(buffer, "%s", compiler->colours.reg); switch (reg->type) { @@ -925,10 +939,6 @@ static void shader_dump_register(struct vkd3d_d3d_asm_compiler *compiler, const shader_addline(buffer, "b"); break; - case VKD3DSPR_LABEL: - shader_addline(buffer, "l"); - break; - case VKD3DSPR_LOOP: shader_addline(buffer, "aL"); break; @@ -1522,6 +1532,7 @@ static void shader_dump_instruction_flags(struct vkd3d_d3d_asm_compiler *compile switch (ins->handler_idx) { + case VKD3DSIH_BRANCH: case VKD3DSIH_BREAKP: case VKD3DSIH_CONTINUEP: case VKD3DSIH_DISCARD: @@ -1646,6 +1657,31 @@ static void shader_dump_instruction(struct vkd3d_d3d_asm_compiler *compiler, switch (ins->handler_idx) { + case VKD3DSIH_BRANCH: + { + const struct vkd3d_shader_src_param *src = ins->src, *merge; + + if (ins->src_count > 2) + { + shader_dump_instruction_flags(compiler, ins); + vkd3d_string_buffer_printf(buffer, " "); + shader_dump_src_param(compiler, &src[0]); + shader_print_label_id(compiler, ", ", src[1].reg.idx[0].offset); + shader_print_label_id(compiler, ", ", src[1].reg.idx[1].offset); + merge = &src[2]; + } + else + { + shader_print_label_id(compiler, " ", src[0].reg.idx[0].offset); + merge = (ins->src_count > 1) ? &src[1] : NULL; + } + if (merge) + { + shader_print_label_id(compiler, ", ", merge->reg.idx[0].offset); + } + break; + } + case VKD3DSIH_DCL: case VKD3DSIH_DCL_UAV_TYPED: vkd3d_string_buffer_printf(buffer, "%s", compiler->colours.opcode); diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index a35ef8620..6aa2bcb3c 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1427,6 +1427,330 @@ static enum vkd3d_result normalise_combined_samplers(struct vkd3d_shader_parser return VKD3D_OK; } +struct cf_flattener_if_info +{ + struct vkd3d_shader_src_param *targets; + unsigned int id; + uint32_t merge_block_id; + unsigned int else_block_id; +}; + +struct cf_flattener_info +{ + union + { + struct cf_flattener_if_info if_; + } u; + + enum + { + VKD3D_BLOCK_IF, + } current_block; + bool inside_block; +}; + +struct cf_flattener +{ + struct vkd3d_shader_parser *parser; + + struct vkd3d_shader_location location; + bool allocation_failed; + + struct vkd3d_shader_instruction *instructions; + size_t instruction_capacity; + size_t instruction_count; + + unsigned int block_id; + + unsigned int branch_id; + unsigned int control_flow_depth; + struct cf_flattener_info *control_flow_info; + size_t control_flow_info_size; +}; + +static struct vkd3d_shader_instruction *cf_flattener_require_space(struct cf_flattener *flattener, size_t count) +{ + if (!vkd3d_array_reserve((void **)&flattener->instructions, &flattener->instruction_capacity, + flattener->instruction_count + count, sizeof(*flattener->instructions))) + { + ERR("Failed to allocate instructions.\n"); + flattener->allocation_failed = true; + return NULL; + } + return &flattener->instructions[flattener->instruction_count]; +} + +static bool cf_flattener_copy_instruction(struct cf_flattener *flattener, + const struct vkd3d_shader_instruction *instruction) +{ + struct vkd3d_shader_instruction *dst_ins; + + if (instruction->handler_idx == VKD3DSIH_NOP) + return true; + + if (!(dst_ins = cf_flattener_require_space(flattener, 1))) + return false; + + *dst_ins = *instruction; + ++flattener->instruction_count; + return true; +} + +static unsigned int cf_flattener_alloc_block_id(struct cf_flattener *flattener) +{ + return ++flattener->block_id; +} + +static struct vkd3d_shader_src_param *instruction_src_params_alloc(struct vkd3d_shader_instruction *ins, + unsigned int count, struct cf_flattener *flattener) +{ + struct vkd3d_shader_src_param *params = shader_parser_get_src_params(flattener->parser, count); + if (!params) + { + flattener->allocation_failed = true; + return NULL; + } + ins->src = params; + ins->src_count = count; + return params; +} + +static void src_param_init_label(struct vkd3d_shader_src_param *param, unsigned int label_id0, unsigned int label_id1) +{ + param->swizzle = 0; + param->modifiers = VKD3DSPSM_NONE; + vsir_register_init(¶m->reg, VKD3DSPR_LABEL, VKD3D_DATA_UINT, 1 + !!label_id1); + param->reg.idx[0].offset = label_id0; + param->reg.idx[1].offset = label_id1; +} + +static void cf_flattener_emit_label(struct cf_flattener *flattener, unsigned int label_id) +{ + struct vkd3d_shader_src_param *src_param; + struct vkd3d_shader_instruction *ins; + + if (!(ins = cf_flattener_require_space(flattener, 1))) + return; + vsir_instruction_init(ins, &flattener->location, VKD3DSIH_LABEL); + + if (!(src_param = instruction_src_params_alloc(ins, 1, flattener))) + return; + src_param_init_label(src_param, label_id, 0); + + ++flattener->instruction_count; +} + +/* For conditional branches, this returns the true+false target branch parameter . */ +static struct vkd3d_shader_src_param *cf_flattener_emit_branch(struct cf_flattener *flattener, + unsigned int merge_block_id, const struct vkd3d_shader_src_param *condition, + unsigned int true_id, unsigned int false_id, + unsigned int flags) +{ + struct vkd3d_shader_src_param *src_params, *target_branch_param; + struct vkd3d_shader_instruction *ins; + + if (!(ins = cf_flattener_require_space(flattener, 1))) + return NULL; + vsir_instruction_init(ins, &flattener->location, VKD3DSIH_BRANCH); + + if (condition) + { + if (!(src_params = instruction_src_params_alloc(ins, 3, flattener))) + return NULL; + src_params[0] = *condition; + src_param_init_label(&src_params[1], true_id, false_id); + src_param_init_label(&src_params[2], merge_block_id, 0); + target_branch_param = &src_params[1]; + } + else + { + if (!(src_params = instruction_src_params_alloc(ins, 1 + !!merge_block_id, flattener))) + return NULL; + src_param_init_label(&src_params[0], true_id, 0); + if (merge_block_id) + src_param_init_label(&src_params[1], merge_block_id, 0); + target_branch_param = NULL; + } + + ins->flags = flags; + ++flattener->instruction_count; + + return target_branch_param; +} + +static void cf_flattener_emit_unconditional_branch(struct cf_flattener *flattener, unsigned int target_block_id) +{ + cf_flattener_emit_branch(flattener, 0, NULL, target_block_id, 0, 0); +} + +static struct cf_flattener_info *cf_flattener_push_control_flow_level(struct cf_flattener *flattener) +{ + if (!vkd3d_array_reserve((void **)&flattener->control_flow_info, &flattener->control_flow_info_size, + flattener->control_flow_depth + 1, sizeof(*flattener->control_flow_info))) + { + ERR("Failed to allocate control flow info structure.\n"); + flattener->allocation_failed = true; + return NULL; + } + + return &flattener->control_flow_info[flattener->control_flow_depth++]; +} + +static void cf_flattener_pop_control_flow_level(struct cf_flattener *flattener) +{ + struct cf_flattener_info *cf_info; + + cf_info = &flattener->control_flow_info[--flattener->control_flow_depth]; + memset(cf_info, 0, sizeof(*cf_info)); +} + +static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flattener *flattener) +{ + struct vkd3d_shader_parser *parser = flattener->parser; + struct vkd3d_shader_instruction_array *instructions; + struct vkd3d_shader_instruction *dst_ins; + unsigned int depth = 0; + bool main_block_open; + size_t i; + + instructions = &parser->instructions; + main_block_open = parser->shader_version.type != VKD3D_SHADER_TYPE_HULL; + + if (!cf_flattener_require_space(flattener, instructions->count)) + return VKD3D_ERROR_OUT_OF_MEMORY; + + for (i = 0; i < instructions->count; ++i) + { + const struct vkd3d_shader_instruction *instruction = &instructions->elements[i]; + const struct vkd3d_shader_src_param *src = instruction->src; + unsigned int merge_block_id, true_block_id; + struct cf_flattener_info *cf_info; + + flattener->location = instruction->location; + + cf_info = flattener->control_flow_depth + ? &flattener->control_flow_info[flattener->control_flow_depth - 1] : NULL; + + switch (instruction->handler_idx) + { + case VKD3DSIH_IF: + if (!(cf_info = cf_flattener_push_control_flow_level(flattener))) + return VKD3D_ERROR_OUT_OF_MEMORY; + + true_block_id = cf_flattener_alloc_block_id(flattener); + merge_block_id = cf_flattener_alloc_block_id(flattener); + cf_info->u.if_.targets = cf_flattener_emit_branch(flattener, merge_block_id, + src, true_block_id, merge_block_id, instruction->flags); + if (!cf_info->u.if_.targets) + return VKD3D_ERROR_OUT_OF_MEMORY; + + cf_flattener_emit_label(flattener, true_block_id); + + cf_info->u.if_.id = flattener->branch_id; + cf_info->u.if_.merge_block_id = merge_block_id; + cf_info->u.if_.else_block_id = 0; + cf_info->inside_block = true; + cf_info->current_block = VKD3D_BLOCK_IF; + + ++flattener->branch_id; + break; + + case VKD3DSIH_ELSE: + if (cf_info->inside_block) + cf_flattener_emit_unconditional_branch(flattener, cf_info->u.if_.merge_block_id); + + cf_info->u.if_.else_block_id = cf_flattener_alloc_block_id(flattener); + cf_info->u.if_.targets->reg.idx[1].offset = cf_info->u.if_.else_block_id; + + cf_flattener_emit_label(flattener, cf_info->u.if_.else_block_id); + + cf_info->inside_block = true; + break; + + case VKD3DSIH_ENDIF: + if (cf_info->inside_block) + cf_flattener_emit_unconditional_branch(flattener, cf_info->u.if_.merge_block_id); + + cf_flattener_emit_label(flattener, cf_info->u.if_.merge_block_id); + + cf_flattener_pop_control_flow_level(flattener); + break; + + case VKD3DSIH_LOOP: + case VKD3DSIH_SWITCH: + ++depth; + if (!cf_flattener_copy_instruction(flattener, instruction)) + return VKD3D_ERROR_OUT_OF_MEMORY; + break; + + case VKD3DSIH_ENDLOOP: + case VKD3DSIH_ENDSWITCH: + --depth; + if (cf_info) + cf_info->inside_block = true; /* Dead code removal ensures this is correct. */ + if (!cf_flattener_copy_instruction(flattener, instruction)) + return VKD3D_ERROR_OUT_OF_MEMORY; + break; + + case VKD3DSIH_RET: + if (!cf_flattener_copy_instruction(flattener, instruction)) + return VKD3D_ERROR_OUT_OF_MEMORY; + + if (cf_info) + cf_info->inside_block = false; + else if (!depth) + main_block_open = false; + break; + + case VKD3DSIH_BREAK: + case VKD3DSIH_CONTINUE: + if (cf_info) + cf_info->inside_block = false; + /* fall through */ + default: + if (!cf_flattener_copy_instruction(flattener, instruction)) + return VKD3D_ERROR_OUT_OF_MEMORY; + break; + } + } + + if (main_block_open) + { + if (!(dst_ins = cf_flattener_require_space(flattener, 1))) + return VKD3D_ERROR_OUT_OF_MEMORY; + vsir_instruction_init(dst_ins, &flattener->location, VKD3DSIH_RET); + ++flattener->instruction_count; + } + + return flattener->allocation_failed ? VKD3D_ERROR_OUT_OF_MEMORY : VKD3D_OK; +} + +static enum vkd3d_result flatten_control_flow_constructs(struct vkd3d_shader_parser *parser) +{ + struct cf_flattener flattener = {0}; + enum vkd3d_result result; + + flattener.parser = parser; + result = cf_flattener_iterate_instruction_array(&flattener); + + if (result >= 0) + { + vkd3d_free(parser->instructions.elements); + parser->instructions.elements = flattener.instructions; + parser->instructions.capacity = flattener.instruction_capacity; + parser->instructions.count = flattener.instruction_count; + parser->shader_desc.block_count = flattener.block_id; + } + else + { + vkd3d_free(flattener.instructions); + } + + vkd3d_free(flattener.control_flow_info); + + return result; +} + enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, const struct vkd3d_shader_compile_info *compile_info) { @@ -1455,6 +1779,9 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, if (result >= 0) remove_dead_code(parser); + if (result >= 0) + result = flatten_control_flow_constructs(parser); + if (result >= 0) result = normalise_combined_samplers(parser); diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index dc295d8ac..5341cd406 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -851,20 +851,6 @@ static void vkd3d_spirv_end_function_stream_insertion(struct vkd3d_spirv_builder builder->insertion_location = ~(size_t)0; } -struct vkd3d_spirv_op_branch_conditional -{ - uint32_t opcode; - uint32_t condition_id; - uint32_t true_label; - uint32_t false_label; -}; - -static struct vkd3d_spirv_op_branch_conditional *vkd3d_spirv_as_op_branch_conditional( - struct vkd3d_spirv_stream *stream, size_t location) -{ - return (struct vkd3d_spirv_op_branch_conditional *)&stream->words[location]; -} - static void vkd3d_spirv_build_op_capability(struct vkd3d_spirv_stream *stream, SpvCapability cap) { @@ -2241,14 +2227,6 @@ static const char *debug_vkd3d_symbol(const struct vkd3d_symbol *symbol) } } -struct vkd3d_if_cf_info -{ - size_t stream_location; - unsigned int id; - uint32_t merge_block_id; - uint32_t else_block_id; -}; - struct vkd3d_loop_cf_info { uint32_t header_block_id; @@ -2272,14 +2250,12 @@ struct vkd3d_control_flow_info { union { - struct vkd3d_if_cf_info if_; struct vkd3d_loop_cf_info loop; struct vkd3d_switch_cf_info switch_; } u; enum { - VKD3D_BLOCK_IF, VKD3D_BLOCK_LOOP, VKD3D_BLOCK_SWITCH, } current_block; @@ -2339,7 +2315,6 @@ struct spirv_compiler enum vkd3d_shader_type shader_type; - unsigned int branch_id; unsigned int loop_id; unsigned int switch_id; unsigned int control_flow_depth; @@ -2354,7 +2329,6 @@ struct spirv_compiler struct vkd3d_push_constant_buffer_binding *push_constants; const struct vkd3d_shader_spirv_target_info *spirv_target_info; - bool main_block_open; bool after_declarations_section; struct shader_signature input_signature; struct shader_signature output_signature; @@ -2393,6 +2367,9 @@ struct spirv_compiler struct ssa_register_info *ssa_register_info; unsigned int ssa_register_count; + + uint32_t *block_label_ids; + unsigned int block_count; }; static bool is_in_default_phase(const struct spirv_compiler *compiler) @@ -2442,6 +2419,7 @@ static void spirv_compiler_destroy(struct spirv_compiler *compiler) shader_signature_cleanup(&compiler->patch_constant_signature); vkd3d_free(compiler->ssa_register_info); + vkd3d_free(compiler->block_label_ids); vkd3d_free(compiler); } @@ -2771,6 +2749,14 @@ static struct vkd3d_string_buffer *vkd3d_shader_register_range_string(struct spi return buffer; } +static uint32_t spirv_compiler_get_label_id(struct spirv_compiler *compiler, unsigned int block_id) +{ + --block_id; + if (!compiler->block_label_ids[block_id]) + compiler->block_label_ids[block_id] = vkd3d_spirv_alloc_id(&compiler->spirv_builder); + return compiler->block_label_ids[block_id]; +} + static struct vkd3d_shader_descriptor_binding spirv_compiler_get_descriptor_binding( struct spirv_compiler *compiler, const struct vkd3d_shader_register *reg, const struct vkd3d_shader_register_range *range, enum vkd3d_shader_resource_type resource_type, @@ -5489,7 +5475,6 @@ static void spirv_compiler_emit_initial_declarations(struct spirv_compiler *comp if (compiler->shader_type != VKD3D_SHADER_TYPE_HULL) { vkd3d_spirv_builder_begin_main_function(builder); - compiler->main_block_open = true; } } @@ -7594,72 +7579,27 @@ static struct vkd3d_control_flow_info *spirv_compiler_find_innermost_breakable_c return NULL; } +static void spirv_compiler_emit_label(struct spirv_compiler *compiler, + const struct vkd3d_shader_instruction *instruction); + static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { uint32_t loop_header_block_id, loop_body_block_id, continue_block_id; struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; const struct vkd3d_shader_src_param *src = instruction->src; - uint32_t merge_block_id, val_id, condition_id, true_label; struct vkd3d_control_flow_info *cf_info; + uint32_t merge_block_id, val_id; cf_info = compiler->control_flow_depth ? &compiler->control_flow_info[compiler->control_flow_depth - 1] : NULL; switch (instruction->handler_idx) { - case VKD3DSIH_IF: - if (!(cf_info = spirv_compiler_push_control_flow_level(compiler))) - return VKD3D_ERROR_OUT_OF_MEMORY; - - val_id = spirv_compiler_emit_load_src(compiler, src, VKD3DSP_WRITEMASK_0); - condition_id = spirv_compiler_emit_int_to_bool(compiler, instruction->flags, 1, val_id); - - true_label = vkd3d_spirv_alloc_id(builder); - merge_block_id = vkd3d_spirv_alloc_id(builder); - vkd3d_spirv_build_op_selection_merge(builder, merge_block_id, SpvSelectionControlMaskNone); - cf_info->u.if_.stream_location = vkd3d_spirv_stream_current_location(&builder->function_stream); - vkd3d_spirv_build_op_branch_conditional(builder, condition_id, true_label, merge_block_id); - - vkd3d_spirv_build_op_label(builder, true_label); - - cf_info->u.if_.id = compiler->branch_id; - cf_info->u.if_.merge_block_id = merge_block_id; - cf_info->u.if_.else_block_id = 0; - cf_info->inside_block = true; - cf_info->current_block = VKD3D_BLOCK_IF; - - vkd3d_spirv_build_op_name(builder, merge_block_id, "branch%u_merge", compiler->branch_id); - vkd3d_spirv_build_op_name(builder, true_label, "branch%u_true", compiler->branch_id); - ++compiler->branch_id; - break; - - case VKD3DSIH_ELSE: - assert(compiler->control_flow_depth); - assert(cf_info->current_block == VKD3D_BLOCK_IF); - - if (cf_info->inside_block) - vkd3d_spirv_build_op_branch(builder, cf_info->u.if_.merge_block_id); - - cf_info->u.if_.else_block_id = vkd3d_spirv_alloc_id(builder); - vkd3d_spirv_as_op_branch_conditional(&builder->function_stream, - cf_info->u.if_.stream_location)->false_label = cf_info->u.if_.else_block_id; - vkd3d_spirv_build_op_name(builder, - cf_info->u.if_.else_block_id, "branch%u_false", cf_info->u.if_.id); - vkd3d_spirv_build_op_label(builder, cf_info->u.if_.else_block_id); - cf_info->inside_block = true; - break; - - case VKD3DSIH_ENDIF: - assert(compiler->control_flow_depth); - assert(cf_info->current_block == VKD3D_BLOCK_IF); - - if (cf_info->inside_block) - vkd3d_spirv_build_op_branch(builder, cf_info->u.if_.merge_block_id); - - vkd3d_spirv_build_op_label(builder, cf_info->u.if_.merge_block_id); - - spirv_compiler_pop_control_flow_level(compiler); + case VKD3DSIH_LABEL: + if (cf_info) + cf_info->inside_block = true; + spirv_compiler_emit_label(compiler, instruction); break; case VKD3DSIH_LOOP: @@ -7896,8 +7836,6 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c if (cf_info) cf_info->inside_block = false; - else - compiler->main_block_open = false; break; default: @@ -7908,6 +7846,71 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c return VKD3D_OK; } +static bool spirv_compiler_init_blocks(struct spirv_compiler *compiler, unsigned int block_count) +{ + compiler->block_count = block_count; + + if (!(compiler->block_label_ids = vkd3d_calloc(block_count, sizeof(*compiler->block_label_ids)))) + return false; + + return true; +} + +static void spirv_compiler_emit_label(struct spirv_compiler *compiler, + const struct vkd3d_shader_instruction *instruction) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + const struct vkd3d_shader_src_param *src = instruction->src; + unsigned int block_id = src->reg.idx[0].offset; + uint32_t label_id; + + label_id = spirv_compiler_get_label_id(compiler, block_id); + vkd3d_spirv_build_op_label(builder, label_id); +} + +static void spirv_compiler_emit_merge(struct spirv_compiler *compiler, + uint32_t merge_block_id) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + + if (!merge_block_id) + return; + + merge_block_id = spirv_compiler_get_label_id(compiler, merge_block_id); + vkd3d_spirv_build_op_selection_merge(builder, merge_block_id, SpvSelectionControlMaskNone); +} + +static void spirv_compiler_emit_branch(struct spirv_compiler *compiler, + const struct vkd3d_shader_instruction *instruction) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + const struct vkd3d_shader_src_param *src = instruction->src; + uint32_t condition_id; + + if (instruction->src_count <= 2) + { + if (instruction->src_count == 2) + spirv_compiler_emit_merge(compiler, src[1].reg.idx[0].offset); + vkd3d_spirv_build_op_branch(builder, spirv_compiler_get_label_id(compiler, src[0].reg.idx[0].offset)); + return; + } + + if (!vkd3d_swizzle_is_scalar(src->swizzle)) + { + WARN("Unexpected src swizzle %#x.\n", src->swizzle); + spirv_compiler_warning(compiler, VKD3D_SHADER_WARNING_SPV_INVALID_SWIZZLE, + "The swizzle for a branch condition value is not scalar."); + } + + condition_id = spirv_compiler_emit_load_src(compiler, &src[0], VKD3DSP_WRITEMASK_0); + condition_id = spirv_compiler_emit_int_to_bool(compiler, instruction->flags, 1, condition_id); + /* Emit the merge immediately before the branch instruction. */ + spirv_compiler_emit_merge(compiler, src[2].reg.idx[0].offset); + vkd3d_spirv_build_op_branch_conditional(builder, condition_id, + spirv_compiler_get_label_id(compiler, src[1].reg.idx[0].offset), + spirv_compiler_get_label_id(compiler, src[1].reg.idx[1].offset)); +} + static void spirv_compiler_emit_deriv_instruction(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { @@ -9667,11 +9670,9 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_CONTINUE: case VKD3DSIH_CONTINUEP: case VKD3DSIH_DEFAULT: - case VKD3DSIH_ELSE: - case VKD3DSIH_ENDIF: case VKD3DSIH_ENDLOOP: case VKD3DSIH_ENDSWITCH: - case VKD3DSIH_IF: + case VKD3DSIH_LABEL: case VKD3DSIH_LOOP: case VKD3DSIH_RET: case VKD3DSIH_SWITCH: @@ -9684,6 +9685,9 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_TEXKILL: spirv_compiler_emit_kill(compiler, instruction); break; + case VKD3DSIH_BRANCH: + spirv_compiler_emit_branch(compiler, instruction); + break; case VKD3DSIH_DSX: case VKD3DSIH_DSX_COARSE: case VKD3DSIH_DSX_FINE: @@ -9875,6 +9879,9 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, if ((result = vkd3d_shader_normalise(parser, compile_info)) < 0) return result; + if (parser->shader_desc.block_count && !spirv_compiler_init_blocks(compiler, parser->shader_desc.block_count)) + return VKD3D_ERROR_OUT_OF_MEMORY; + instructions = parser->instructions; memset(&parser->instructions, 0, sizeof(parser->instructions)); @@ -9903,9 +9910,6 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, if (result < 0) return result; - if (compiler->main_block_open) - vkd3d_spirv_build_op_return(builder); - if (!is_in_default_phase(compiler)) spirv_compiler_leave_shader_phase(compiler); else diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 102d49a38..bda069503 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -236,6 +236,7 @@ enum vkd3d_shader_opcode VKD3DSIH_BEM, VKD3DSIH_BFI, VKD3DSIH_BFREV, + VKD3DSIH_BRANCH, VKD3DSIH_BREAK, VKD3DSIH_BREAKC, VKD3DSIH_BREAKP, @@ -1005,6 +1006,7 @@ struct vkd3d_shader_desc uint32_t temp_count; unsigned int ssa_count; + unsigned int block_count; struct { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/d3d_asm.c | 2 + libs/vkd3d-shader/ir.c | 192 ++++++++++++++++++++++++++++++++---- libs/vkd3d-shader/spirv.c | 147 +++------------------------ 3 files changed, 187 insertions(+), 154 deletions(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index 0b13b8c0a..a43970165 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -1678,6 +1678,8 @@ static void shader_dump_instruction(struct vkd3d_d3d_asm_compiler *compiler, if (merge) { shader_print_label_id(compiler, ", ", merge->reg.idx[0].offset); + if (merge->reg.idx_count > 1) + shader_print_label_id(compiler, ", ", merge->reg.idx[1].offset); } break; } diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 6aa2bcb3c..85faca705 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1435,16 +1435,26 @@ struct cf_flattener_if_info unsigned int else_block_id; }; +struct cf_flattener_loop_info +{ + unsigned int header_block_id; + unsigned int continue_block_id; + uint32_t merge_block_id; +}; + struct cf_flattener_info { union { struct cf_flattener_if_info if_; + struct cf_flattener_loop_info loop; } u; enum { VKD3D_BLOCK_IF, + VKD3D_BLOCK_LOOP, + VKD3D_BLOCK_SWITCH, } current_block; bool inside_block; }; @@ -1463,6 +1473,7 @@ struct cf_flattener unsigned int block_id; unsigned int branch_id; + unsigned int loop_id; unsigned int control_flow_depth; struct cf_flattener_info *control_flow_info; size_t control_flow_info_size; @@ -1542,8 +1553,8 @@ static void cf_flattener_emit_label(struct cf_flattener *flattener, unsigned int /* For conditional branches, this returns the true+false target branch parameter . */ static struct vkd3d_shader_src_param *cf_flattener_emit_branch(struct cf_flattener *flattener, - unsigned int merge_block_id, const struct vkd3d_shader_src_param *condition, - unsigned int true_id, unsigned int false_id, + unsigned int merge_block_id, unsigned int continue_block_id, + const struct vkd3d_shader_src_param *condition, unsigned int true_id, unsigned int false_id, unsigned int flags) { struct vkd3d_shader_src_param *src_params, *target_branch_param; @@ -1559,16 +1570,16 @@ static struct vkd3d_shader_src_param *cf_flattener_emit_branch(struct cf_flatten return NULL; src_params[0] = *condition; src_param_init_label(&src_params[1], true_id, false_id); - src_param_init_label(&src_params[2], merge_block_id, 0); + src_param_init_label(&src_params[2], merge_block_id, continue_block_id); target_branch_param = &src_params[1]; } else { - if (!(src_params = instruction_src_params_alloc(ins, 1 + !!merge_block_id, flattener))) + if (!(src_params = instruction_src_params_alloc(ins, 1 + (merge_block_id || continue_block_id), flattener))) return NULL; src_param_init_label(&src_params[0], true_id, 0); - if (merge_block_id) - src_param_init_label(&src_params[1], merge_block_id, 0); + if (merge_block_id || continue_block_id) + src_param_init_label(&src_params[1], merge_block_id, continue_block_id); target_branch_param = NULL; } @@ -1578,9 +1589,19 @@ static struct vkd3d_shader_src_param *cf_flattener_emit_branch(struct cf_flatten return target_branch_param; } +static void cf_flattener_emit_conditional_branch_and_merge(struct cf_flattener *flattener, + const struct vkd3d_shader_src_param *condition, unsigned int true_id, unsigned int flags) +{ + unsigned int merge_block_id; + + merge_block_id = cf_flattener_alloc_block_id(flattener); + cf_flattener_emit_branch(flattener, merge_block_id, 0, condition, true_id, merge_block_id, flags); + cf_flattener_emit_label(flattener, merge_block_id); +} + static void cf_flattener_emit_unconditional_branch(struct cf_flattener *flattener, unsigned int target_block_id) { - cf_flattener_emit_branch(flattener, 0, NULL, target_block_id, 0, 0); + cf_flattener_emit_branch(flattener, 0, 0, NULL, target_block_id, 0, 0); } static struct cf_flattener_info *cf_flattener_push_control_flow_level(struct cf_flattener *flattener) @@ -1604,12 +1625,38 @@ static void cf_flattener_pop_control_flow_level(struct cf_flattener *flattener) memset(cf_info, 0, sizeof(*cf_info)); } +static struct cf_flattener_info *cf_flattener_find_innermost_loop(struct cf_flattener *flattener) +{ + int depth; + + for (depth = flattener->control_flow_depth - 1; depth >= 0; --depth) + { + if (flattener->control_flow_info[depth].current_block == VKD3D_BLOCK_LOOP) + return &flattener->control_flow_info[depth]; + } + + return NULL; +} + +static struct cf_flattener_info *cf_flattener_find_innermost_breakable_cf_construct(struct cf_flattener *flattener) +{ + int depth; + + for (depth = flattener->control_flow_depth - 1; depth >= 0; --depth) + { + if (flattener->control_flow_info[depth].current_block == VKD3D_BLOCK_LOOP + || flattener->control_flow_info[depth].current_block == VKD3D_BLOCK_SWITCH) + return &flattener->control_flow_info[depth]; + } + + return NULL; +} + static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flattener *flattener) { struct vkd3d_shader_parser *parser = flattener->parser; struct vkd3d_shader_instruction_array *instructions; struct vkd3d_shader_instruction *dst_ins; - unsigned int depth = 0; bool main_block_open; size_t i; @@ -1621,9 +1668,9 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte for (i = 0; i < instructions->count; ++i) { + unsigned int loop_header_block_id, loop_body_block_id, continue_block_id, merge_block_id, true_block_id; const struct vkd3d_shader_instruction *instruction = &instructions->elements[i]; const struct vkd3d_shader_src_param *src = instruction->src; - unsigned int merge_block_id, true_block_id; struct cf_flattener_info *cf_info; flattener->location = instruction->location; @@ -1639,7 +1686,7 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte true_block_id = cf_flattener_alloc_block_id(flattener); merge_block_id = cf_flattener_alloc_block_id(flattener); - cf_info->u.if_.targets = cf_flattener_emit_branch(flattener, merge_block_id, + cf_info->u.if_.targets = cf_flattener_emit_branch(flattener, merge_block_id, 0, src, true_block_id, merge_block_id, instruction->flags); if (!cf_info->u.if_.targets) return VKD3D_ERROR_OUT_OF_MEMORY; @@ -1677,36 +1724,139 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte break; case VKD3DSIH_LOOP: + if (!(cf_info = cf_flattener_push_control_flow_level(flattener))) + return VKD3D_ERROR_OUT_OF_MEMORY; + + loop_header_block_id = cf_flattener_alloc_block_id(flattener); + loop_body_block_id = cf_flattener_alloc_block_id(flattener); + continue_block_id = cf_flattener_alloc_block_id(flattener); + merge_block_id = cf_flattener_alloc_block_id(flattener); + + cf_flattener_emit_unconditional_branch(flattener, loop_header_block_id); + cf_flattener_emit_label(flattener, loop_header_block_id); + cf_flattener_emit_branch(flattener, merge_block_id, continue_block_id, + NULL, loop_body_block_id, 0, 0); + + cf_flattener_emit_label(flattener, loop_body_block_id); + + cf_info->u.loop.header_block_id = loop_header_block_id; + cf_info->u.loop.continue_block_id = continue_block_id; + cf_info->u.loop.merge_block_id = merge_block_id; + cf_info->current_block = VKD3D_BLOCK_LOOP; + cf_info->inside_block = true; + + ++flattener->loop_id; + break; + + case VKD3DSIH_ENDLOOP: + if (cf_info->inside_block) + cf_flattener_emit_unconditional_branch(flattener, cf_info->u.loop.continue_block_id); + + cf_flattener_emit_label(flattener, cf_info->u.loop.continue_block_id); + cf_flattener_emit_unconditional_branch(flattener, cf_info->u.loop.header_block_id); + cf_flattener_emit_label(flattener, cf_info->u.loop.merge_block_id); + + cf_flattener_pop_control_flow_level(flattener); + break; + case VKD3DSIH_SWITCH: - ++depth; + if (!(cf_info = cf_flattener_push_control_flow_level(flattener))) + return VKD3D_ERROR_OUT_OF_MEMORY; + if (!cf_flattener_copy_instruction(flattener, instruction)) return VKD3D_ERROR_OUT_OF_MEMORY; + + cf_info->current_block = VKD3D_BLOCK_SWITCH; + break; - case VKD3DSIH_ENDLOOP: case VKD3DSIH_ENDSWITCH: - --depth; - if (cf_info) - cf_info->inside_block = true; /* Dead code removal ensures this is correct. */ + cf_flattener_pop_control_flow_level(flattener); + if (!cf_flattener_copy_instruction(flattener, instruction)) return VKD3D_ERROR_OUT_OF_MEMORY; break; + case VKD3DSIH_BREAK: + { + struct cf_flattener_info *breakable_cf_info; + + if (!(breakable_cf_info = cf_flattener_find_innermost_breakable_cf_construct(flattener))) + { + FIXME("Unhandled break instruction.\n"); + return VKD3D_ERROR_INVALID_SHADER; + } + + if (breakable_cf_info->current_block == VKD3D_BLOCK_LOOP) + { + cf_flattener_emit_unconditional_branch(flattener, breakable_cf_info->u.loop.merge_block_id); + } + else if (breakable_cf_info->current_block == VKD3D_BLOCK_SWITCH) + { + if (!cf_flattener_copy_instruction(flattener, instruction)) + return VKD3D_ERROR_OUT_OF_MEMORY; + } + + cf_info->inside_block = false; + break; + } + + case VKD3DSIH_BREAKP: + { + struct cf_flattener_info *loop_cf_info; + + if (!(loop_cf_info = cf_flattener_find_innermost_loop(flattener))) + { + ERR("Invalid 'breakc' instruction outside loop.\n"); + return VKD3D_ERROR_INVALID_SHADER; + } + + cf_flattener_emit_conditional_branch_and_merge(flattener, + src, loop_cf_info->u.loop.merge_block_id, instruction->flags); + break; + } + + case VKD3DSIH_CONTINUE: + { + struct cf_flattener_info *loop_cf_info; + + if (!(loop_cf_info = cf_flattener_find_innermost_loop(flattener))) + { + ERR("Invalid 'continue' instruction outside loop.\n"); + return VKD3D_ERROR_INVALID_SHADER; + } + + cf_flattener_emit_unconditional_branch(flattener, loop_cf_info->u.loop.continue_block_id); + + cf_info->inside_block = false; + break; + } + + case VKD3DSIH_CONTINUEP: + { + struct cf_flattener_info *loop_cf_info; + + if (!(loop_cf_info = cf_flattener_find_innermost_loop(flattener))) + { + ERR("Invalid 'continuec' instruction outside loop.\n"); + return VKD3D_ERROR_INVALID_SHADER; + } + + cf_flattener_emit_conditional_branch_and_merge(flattener, + src, loop_cf_info->u.loop.continue_block_id, instruction->flags); + break; + } + case VKD3DSIH_RET: if (!cf_flattener_copy_instruction(flattener, instruction)) return VKD3D_ERROR_OUT_OF_MEMORY; if (cf_info) cf_info->inside_block = false; - else if (!depth) + else main_block_open = false; break; - case VKD3DSIH_BREAK: - case VKD3DSIH_CONTINUE: - if (cf_info) - cf_info->inside_block = false; - /* fall through */ default: if (!cf_flattener_copy_instruction(flattener, instruction)) return VKD3D_ERROR_OUT_OF_MEMORY; diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 5341cd406..abe508cd1 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2227,13 +2227,6 @@ static const char *debug_vkd3d_symbol(const struct vkd3d_symbol *symbol) } } -struct vkd3d_loop_cf_info -{ - uint32_t header_block_id; - uint32_t continue_block_id; - uint32_t merge_block_id; -}; - struct vkd3d_switch_cf_info { size_t stream_location; @@ -2250,13 +2243,11 @@ struct vkd3d_control_flow_info { union { - struct vkd3d_loop_cf_info loop; struct vkd3d_switch_cf_info switch_; } u; enum { - VKD3D_BLOCK_LOOP, VKD3D_BLOCK_SWITCH, } current_block; bool inside_block; @@ -2315,7 +2306,6 @@ struct spirv_compiler enum vkd3d_shader_type shader_type; - unsigned int loop_id; unsigned int switch_id; unsigned int control_flow_depth; struct vkd3d_control_flow_info *control_flow_info; @@ -7550,20 +7540,6 @@ static void spirv_compiler_pop_control_flow_level(struct spirv_compiler *compile memset(cf_info, 0, sizeof(*cf_info)); } -static struct vkd3d_control_flow_info *spirv_compiler_find_innermost_loop( - struct spirv_compiler *compiler) -{ - int depth; - - for (depth = compiler->control_flow_depth - 1; depth >= 0; --depth) - { - if (compiler->control_flow_info[depth].current_block == VKD3D_BLOCK_LOOP) - return &compiler->control_flow_info[depth]; - } - - return NULL; -} - static struct vkd3d_control_flow_info *spirv_compiler_find_innermost_breakable_cf_construct( struct spirv_compiler *compiler) { @@ -7571,8 +7547,7 @@ static struct vkd3d_control_flow_info *spirv_compiler_find_innermost_breakable_c for (depth = compiler->control_flow_depth - 1; depth >= 0; --depth) { - if (compiler->control_flow_info[depth].current_block == VKD3D_BLOCK_LOOP - || compiler->control_flow_info[depth].current_block == VKD3D_BLOCK_SWITCH) + if (compiler->control_flow_info[depth].current_block == VKD3D_BLOCK_SWITCH) return &compiler->control_flow_info[depth]; } @@ -7585,7 +7560,6 @@ static void spirv_compiler_emit_label(struct spirv_compiler *compiler, static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { - uint32_t loop_header_block_id, loop_body_block_id, continue_block_id; struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; const struct vkd3d_shader_src_param *src = instruction->src; struct vkd3d_control_flow_info *cf_info; @@ -7602,51 +7576,6 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c spirv_compiler_emit_label(compiler, instruction); break; - case VKD3DSIH_LOOP: - if (!(cf_info = spirv_compiler_push_control_flow_level(compiler))) - return VKD3D_ERROR_OUT_OF_MEMORY; - - loop_header_block_id = vkd3d_spirv_alloc_id(builder); - loop_body_block_id = vkd3d_spirv_alloc_id(builder); - continue_block_id = vkd3d_spirv_alloc_id(builder); - merge_block_id = vkd3d_spirv_alloc_id(builder); - - vkd3d_spirv_build_op_branch(builder, loop_header_block_id); - vkd3d_spirv_build_op_label(builder, loop_header_block_id); - vkd3d_spirv_build_op_loop_merge(builder, merge_block_id, continue_block_id, SpvLoopControlMaskNone); - vkd3d_spirv_build_op_branch(builder, loop_body_block_id); - - vkd3d_spirv_build_op_label(builder, loop_body_block_id); - - cf_info->u.loop.header_block_id = loop_header_block_id; - cf_info->u.loop.continue_block_id = continue_block_id; - cf_info->u.loop.merge_block_id = merge_block_id; - cf_info->current_block = VKD3D_BLOCK_LOOP; - cf_info->inside_block = true; - - vkd3d_spirv_build_op_name(builder, loop_header_block_id, "loop%u_header", compiler->loop_id); - vkd3d_spirv_build_op_name(builder, loop_body_block_id, "loop%u_body", compiler->loop_id); - vkd3d_spirv_build_op_name(builder, continue_block_id, "loop%u_continue", compiler->loop_id); - vkd3d_spirv_build_op_name(builder, merge_block_id, "loop%u_merge", compiler->loop_id); - ++compiler->loop_id; - break; - - case VKD3DSIH_ENDLOOP: - assert(compiler->control_flow_depth); - assert(cf_info->current_block == VKD3D_BLOCK_LOOP); - - /* The loop block may have already been ended by an unconditional - * break instruction right before the end of the loop. */ - if (cf_info->inside_block) - vkd3d_spirv_build_op_branch(builder, cf_info->u.loop.continue_block_id); - - vkd3d_spirv_build_op_label(builder, cf_info->u.loop.continue_block_id); - vkd3d_spirv_build_op_branch(builder, cf_info->u.loop.header_block_id); - vkd3d_spirv_build_op_label(builder, cf_info->u.loop.merge_block_id); - - spirv_compiler_pop_control_flow_level(compiler); - break; - case VKD3DSIH_SWITCH: if (!(cf_info = spirv_compiler_push_control_flow_level(compiler))) return VKD3D_ERROR_OUT_OF_MEMORY; @@ -7763,11 +7692,7 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c return VKD3D_ERROR_INVALID_SHADER; } - if (breakable_cf_info->current_block == VKD3D_BLOCK_LOOP) - { - vkd3d_spirv_build_op_branch(builder, breakable_cf_info->u.loop.merge_block_id); - } - else if (breakable_cf_info->current_block == VKD3D_BLOCK_SWITCH) + if (breakable_cf_info->current_block == VKD3D_BLOCK_SWITCH) { /* The current case block may have already been ended by an * unconditional continue instruction. */ @@ -7779,58 +7704,6 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c break; } - case VKD3DSIH_BREAKP: - { - struct vkd3d_control_flow_info *loop_cf_info; - - assert(compiler->control_flow_depth); - - if (!(loop_cf_info = spirv_compiler_find_innermost_loop(compiler))) - { - ERR("Invalid 'breakc' instruction outside loop.\n"); - return VKD3D_ERROR_INVALID_SHADER; - } - - merge_block_id = spirv_compiler_emit_conditional_branch(compiler, - instruction, loop_cf_info->u.loop.merge_block_id); - vkd3d_spirv_build_op_label(builder, merge_block_id); - break; - } - - case VKD3DSIH_CONTINUE: - { - struct vkd3d_control_flow_info *loop_cf_info; - - assert(compiler->control_flow_depth); - - if (!(loop_cf_info = spirv_compiler_find_innermost_loop(compiler))) - { - ERR("Invalid 'continue' instruction outside loop.\n"); - return VKD3D_ERROR_INVALID_SHADER; - } - - vkd3d_spirv_build_op_branch(builder, loop_cf_info->u.loop.continue_block_id); - - cf_info->inside_block = false; - break; - } - - case VKD3DSIH_CONTINUEP: - { - struct vkd3d_control_flow_info *loop_cf_info; - - if (!(loop_cf_info = spirv_compiler_find_innermost_loop(compiler))) - { - ERR("Invalid 'continuec' instruction outside loop.\n"); - return VKD3D_ERROR_INVALID_SHADER; - } - - merge_block_id = spirv_compiler_emit_conditional_branch(compiler, - instruction, loop_cf_info->u.loop.continue_block_id); - vkd3d_spirv_build_op_label(builder, merge_block_id); - break; - } - case VKD3DSIH_RET: spirv_compiler_emit_return(compiler, instruction); @@ -7869,7 +7742,7 @@ static void spirv_compiler_emit_label(struct spirv_compiler *compiler, } static void spirv_compiler_emit_merge(struct spirv_compiler *compiler, - uint32_t merge_block_id) + uint32_t merge_block_id, uint32_t continue_block_id) { struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; @@ -7877,7 +7750,15 @@ static void spirv_compiler_emit_merge(struct spirv_compiler *compiler, return; merge_block_id = spirv_compiler_get_label_id(compiler, merge_block_id); - vkd3d_spirv_build_op_selection_merge(builder, merge_block_id, SpvSelectionControlMaskNone); + if (!continue_block_id) + { + vkd3d_spirv_build_op_selection_merge(builder, merge_block_id, SpvSelectionControlMaskNone); + } + else + { + continue_block_id = spirv_compiler_get_label_id(compiler, continue_block_id); + vkd3d_spirv_build_op_loop_merge(builder, merge_block_id, continue_block_id, SpvLoopControlMaskNone); + } } static void spirv_compiler_emit_branch(struct spirv_compiler *compiler, @@ -7890,7 +7771,7 @@ static void spirv_compiler_emit_branch(struct spirv_compiler *compiler, if (instruction->src_count <= 2) { if (instruction->src_count == 2) - spirv_compiler_emit_merge(compiler, src[1].reg.idx[0].offset); + spirv_compiler_emit_merge(compiler, src[1].reg.idx[0].offset, src[1].reg.idx[1].offset); vkd3d_spirv_build_op_branch(builder, spirv_compiler_get_label_id(compiler, src[0].reg.idx[0].offset)); return; } @@ -7905,7 +7786,7 @@ static void spirv_compiler_emit_branch(struct spirv_compiler *compiler, condition_id = spirv_compiler_emit_load_src(compiler, &src[0], VKD3DSP_WRITEMASK_0); condition_id = spirv_compiler_emit_int_to_bool(compiler, instruction->flags, 1, condition_id); /* Emit the merge immediately before the branch instruction. */ - spirv_compiler_emit_merge(compiler, src[2].reg.idx[0].offset); + spirv_compiler_emit_merge(compiler, src[2].reg.idx[0].offset, src[2].reg.idx[1].offset); vkd3d_spirv_build_op_branch_conditional(builder, condition_id, spirv_compiler_get_label_id(compiler, src[1].reg.idx[0].offset), spirv_compiler_get_label_id(compiler, src[1].reg.idx[1].offset)); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/d3d_asm.c | 17 ++ libs/vkd3d-shader/ir.c | 113 ++++++++- libs/vkd3d-shader/spirv.c | 291 ++++------------------- libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 4 files changed, 170 insertions(+), 252 deletions(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index a43970165..b5544c778 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -279,6 +279,7 @@ static const char * const shader_opcode_names[] = [VKD3DSIH_SUB ] = "sub", [VKD3DSIH_SWAPC ] = "swapc", [VKD3DSIH_SWITCH ] = "switch", + [VKD3DSIH_SWITCH_MONOLITHIC ] = "switch", [VKD3DSIH_SYNC ] = "sync", [VKD3DSIH_TEX ] = "texld", [VKD3DSIH_TEXBEM ] = "texbem", @@ -1894,6 +1895,22 @@ static void shader_dump_instruction(struct vkd3d_d3d_asm_compiler *compiler, shader_print_bool_literal(compiler, " = ", ins->src[0].reg.u.immconst_uint[0], ""); break; + case VKD3DSIH_SWITCH_MONOLITHIC: + { + const struct vkd3d_shader_src_param *src = ins->src; + + vkd3d_string_buffer_printf(buffer, " "); + shader_dump_src_param(compiler, &src[0]); + for (i = 2; i < ins->src_count; ++i) + { + shader_print_int_literal(compiler, ", ", src[i].reg.u.immconst_uint[0], ":"); + shader_print_label_id(compiler, " ", src[i].reg.idx[0].offset); + } + shader_print_label_id(compiler, ", default: ", src[1].reg.idx[0].offset); + shader_print_label_id(compiler, ", merge: ", src[1].reg.idx[1].offset); + break; + } + default: shader_dump_instruction_flags(compiler, ins); diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 85faca705..0c3dad238 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1442,12 +1442,31 @@ struct cf_flattener_loop_info uint32_t merge_block_id; }; +struct cf_flattener_switch_case +{ + unsigned int value; + unsigned int block_id; +}; + +struct cf_flattener_switch_info +{ + size_t ins_location; + const struct vkd3d_shader_src_param *condition; + unsigned int id; + unsigned int merge_block_id; + unsigned int default_block_id; + struct cf_flattener_switch_case *cases; + size_t cases_size; + unsigned int cases_count; +}; + struct cf_flattener_info { union { struct cf_flattener_if_info if_; struct cf_flattener_loop_info loop; + struct cf_flattener_switch_info switch_; } u; enum @@ -1474,6 +1493,7 @@ struct cf_flattener unsigned int branch_id; unsigned int loop_id; + unsigned int switch_id; unsigned int control_flow_depth; struct cf_flattener_info *control_flow_info; size_t control_flow_info_size; @@ -1763,18 +1783,102 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte if (!(cf_info = cf_flattener_push_control_flow_level(flattener))) return VKD3D_ERROR_OUT_OF_MEMORY; - if (!cf_flattener_copy_instruction(flattener, instruction)) - return VKD3D_ERROR_OUT_OF_MEMORY; + merge_block_id = cf_flattener_alloc_block_id(flattener); + cf_info->u.switch_.ins_location = flattener->instruction_count; + cf_info->u.switch_.condition = src; + + if (!(dst_ins = cf_flattener_require_space(flattener, 1))) + return VKD3D_ERROR_OUT_OF_MEMORY; + vsir_instruction_init(dst_ins, &instruction->location, VKD3DSIH_SWITCH_MONOLITHIC); + ++flattener->instruction_count; + + cf_info->u.switch_.id = flattener->switch_id; + cf_info->u.switch_.merge_block_id = merge_block_id; + cf_info->u.switch_.cases = NULL; + cf_info->u.switch_.cases_size = 0; + cf_info->u.switch_.cases_count = 0; + cf_info->u.switch_.default_block_id = 0; + cf_info->inside_block = false; cf_info->current_block = VKD3D_BLOCK_SWITCH; + ++flattener->switch_id; + + if (!vkd3d_array_reserve((void **)&cf_info->u.switch_.cases, &cf_info->u.switch_.cases_size, + 10, sizeof(*cf_info->u.switch_.cases))) + return VKD3D_ERROR_OUT_OF_MEMORY; + break; case VKD3DSIH_ENDSWITCH: + { + struct vkd3d_shader_src_param *src_params; + unsigned int j; + + if (!cf_info->u.switch_.default_block_id) + cf_info->u.switch_.default_block_id = cf_info->u.switch_.merge_block_id; + + cf_flattener_emit_label(flattener, cf_info->u.switch_.merge_block_id); + + /* The SWITCH instruction is completed when the endswitch + * instruction is processed because we do not know the number + * of case statements or the default block id in advance.*/ + dst_ins = &flattener->instructions[cf_info->u.switch_.ins_location]; + if (!(src_params = instruction_src_params_alloc(dst_ins, cf_info->u.switch_.cases_count + 2, flattener))) + { + vkd3d_free(cf_info->u.switch_.cases); + return VKD3D_ERROR_OUT_OF_MEMORY; + } + src_params[0] = *cf_info->u.switch_.condition; + src_param_init_label(&src_params[1], cf_info->u.switch_.default_block_id, + cf_info->u.switch_.merge_block_id); + for (j = 0; j < cf_info->u.switch_.cases_count; ++j) + { + src_param_init_label(&src_params[j + 2], cf_info->u.switch_.cases[j].block_id, 0); + src_params[j + 2].reg.u.immconst_uint[0] = cf_info->u.switch_.cases[j].value; + } + vkd3d_free(cf_info->u.switch_.cases); + cf_flattener_pop_control_flow_level(flattener); + break; + } - if (!cf_flattener_copy_instruction(flattener, instruction)) + case VKD3DSIH_CASE: + { + unsigned int label_id, value; + + if (src->swizzle != VKD3D_SHADER_SWIZZLE(X, X, X, X)) + { + WARN("Unexpected src swizzle %#x.\n", src->swizzle); + vkd3d_shader_parser_error(parser, VKD3D_SHADER_ERROR_VSIR_INVALID_SWIZZLE, + "The swizzle for a switch case value is not scalar X."); + } + value = *src->reg.u.immconst_uint; + + if (!vkd3d_array_reserve((void **)&cf_info->u.switch_.cases, &cf_info->u.switch_.cases_size, + cf_info->u.switch_.cases_count + 1, sizeof(*cf_info->u.switch_.cases))) return VKD3D_ERROR_OUT_OF_MEMORY; + + label_id = cf_flattener_alloc_block_id(flattener); + if (cf_info->inside_block) /* fall-through */ + cf_flattener_emit_unconditional_branch(flattener, label_id); + + cf_info->u.switch_.cases[cf_info->u.switch_.cases_count].value = value; + cf_info->u.switch_.cases[cf_info->u.switch_.cases_count].block_id = label_id; + ++cf_info->u.switch_.cases_count; + + cf_flattener_emit_label(flattener, label_id); + cf_info->inside_block = true; + break; + } + + case VKD3DSIH_DEFAULT: + cf_info->u.switch_.default_block_id = cf_flattener_alloc_block_id(flattener); + if (cf_info->inside_block) /* fall-through */ + cf_flattener_emit_unconditional_branch(flattener, cf_info->u.switch_.default_block_id); + + cf_flattener_emit_label(flattener, cf_info->u.switch_.default_block_id); + cf_info->inside_block = true; break; case VKD3DSIH_BREAK: @@ -1793,8 +1897,7 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte } else if (breakable_cf_info->current_block == VKD3D_BLOCK_SWITCH) { - if (!cf_flattener_copy_instruction(flattener, instruction)) - return VKD3D_ERROR_OUT_OF_MEMORY; + cf_flattener_emit_unconditional_branch(flattener, breakable_cf_info->u.switch_.merge_block_id); } cf_info->inside_block = false; diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index abe508cd1..4a9d18db6 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -348,6 +348,12 @@ static bool vkd3d_spirv_stream_append(struct vkd3d_spirv_stream *dst_stream, return true; } +static void vkd3d_spirv_stream_add_word_count(struct vkd3d_spirv_stream *stream, size_t location, + unsigned int word_count) +{ + stream->words[location] += word_count << SpvWordCountShift; +} + struct vkd3d_spirv_builder { uint64_t capability_mask; @@ -2227,32 +2233,6 @@ static const char *debug_vkd3d_symbol(const struct vkd3d_symbol *symbol) } } -struct vkd3d_switch_cf_info -{ - size_t stream_location; - unsigned int id; - uint32_t selector_id; - uint32_t merge_block_id; - uint32_t default_block_id; - uint32_t *case_blocks; - size_t case_blocks_size; - unsigned int case_block_count; -}; - -struct vkd3d_control_flow_info -{ - union - { - struct vkd3d_switch_cf_info switch_; - } u; - - enum - { - VKD3D_BLOCK_SWITCH, - } current_block; - bool inside_block; -}; - struct vkd3d_push_constant_buffer_binding { struct vkd3d_shader_register reg; @@ -2306,11 +2286,6 @@ struct spirv_compiler enum vkd3d_shader_type shader_type; - unsigned int switch_id; - unsigned int control_flow_depth; - struct vkd3d_control_flow_info *control_flow_info; - size_t control_flow_info_size; - struct vkd3d_shader_interface_info shader_interface; struct vkd3d_shader_descriptor_offset_info offset_info; uint32_t descriptor_offsets_member_id; @@ -2389,8 +2364,6 @@ static const char *spirv_compiler_get_entry_point_name(const struct spirv_compil static void spirv_compiler_destroy(struct spirv_compiler *compiler) { - vkd3d_free(compiler->control_flow_info); - vkd3d_free(compiler->output_info); vkd3d_free(compiler->push_constants); @@ -7517,208 +7490,6 @@ static void spirv_compiler_emit_kill(struct spirv_compiler *compiler, vkd3d_spirv_build_op_label(builder, merge_block_id); } -static struct vkd3d_control_flow_info *spirv_compiler_push_control_flow_level( - struct spirv_compiler *compiler) -{ - if (!vkd3d_array_reserve((void **)&compiler->control_flow_info, &compiler->control_flow_info_size, - compiler->control_flow_depth + 1, sizeof(*compiler->control_flow_info))) - { - ERR("Failed to allocate control flow info structure.\n"); - return NULL; - } - - return &compiler->control_flow_info[compiler->control_flow_depth++]; -} - -static void spirv_compiler_pop_control_flow_level(struct spirv_compiler *compiler) -{ - struct vkd3d_control_flow_info *cf_info; - - assert(compiler->control_flow_depth); - - cf_info = &compiler->control_flow_info[--compiler->control_flow_depth]; - memset(cf_info, 0, sizeof(*cf_info)); -} - -static struct vkd3d_control_flow_info *spirv_compiler_find_innermost_breakable_cf_construct( - struct spirv_compiler *compiler) -{ - int depth; - - for (depth = compiler->control_flow_depth - 1; depth >= 0; --depth) - { - if (compiler->control_flow_info[depth].current_block == VKD3D_BLOCK_SWITCH) - return &compiler->control_flow_info[depth]; - } - - return NULL; -} - -static void spirv_compiler_emit_label(struct spirv_compiler *compiler, - const struct vkd3d_shader_instruction *instruction); - -static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *compiler, - const struct vkd3d_shader_instruction *instruction) -{ - struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; - const struct vkd3d_shader_src_param *src = instruction->src; - struct vkd3d_control_flow_info *cf_info; - uint32_t merge_block_id, val_id; - - cf_info = compiler->control_flow_depth - ? &compiler->control_flow_info[compiler->control_flow_depth - 1] : NULL; - - switch (instruction->handler_idx) - { - case VKD3DSIH_LABEL: - if (cf_info) - cf_info->inside_block = true; - spirv_compiler_emit_label(compiler, instruction); - break; - - case VKD3DSIH_SWITCH: - if (!(cf_info = spirv_compiler_push_control_flow_level(compiler))) - return VKD3D_ERROR_OUT_OF_MEMORY; - - merge_block_id = vkd3d_spirv_alloc_id(builder); - - assert(src->reg.data_type == VKD3D_DATA_INT); - val_id = spirv_compiler_emit_load_src(compiler, src, VKD3DSP_WRITEMASK_0); - - vkd3d_spirv_build_op_selection_merge(builder, merge_block_id, SpvSelectionControlMaskNone); - - cf_info->u.switch_.id = compiler->switch_id; - cf_info->u.switch_.merge_block_id = merge_block_id; - cf_info->u.switch_.stream_location = vkd3d_spirv_stream_current_location(&builder->function_stream); - cf_info->u.switch_.selector_id = val_id; - cf_info->u.switch_.case_blocks = NULL; - cf_info->u.switch_.case_blocks_size = 0; - cf_info->u.switch_.case_block_count = 0; - cf_info->u.switch_.default_block_id = 0; - cf_info->inside_block = false; - cf_info->current_block = VKD3D_BLOCK_SWITCH; - - vkd3d_spirv_build_op_name(builder, merge_block_id, "switch%u_merge", compiler->switch_id); - - ++compiler->switch_id; - - if (!vkd3d_array_reserve((void **)&cf_info->u.switch_.case_blocks, &cf_info->u.switch_.case_blocks_size, - 10, sizeof(*cf_info->u.switch_.case_blocks))) - return VKD3D_ERROR_OUT_OF_MEMORY; - - break; - - case VKD3DSIH_ENDSWITCH: - assert(compiler->control_flow_depth); - assert(cf_info->current_block == VKD3D_BLOCK_SWITCH); - assert(!cf_info->inside_block); - - if (!cf_info->u.switch_.default_block_id) - cf_info->u.switch_.default_block_id = cf_info->u.switch_.merge_block_id; - - vkd3d_spirv_build_op_label(builder, cf_info->u.switch_.merge_block_id); - - /* The OpSwitch instruction is inserted when the endswitch - * instruction is processed because we do not know the number - * of case statements in advance.*/ - vkd3d_spirv_begin_function_stream_insertion(builder, cf_info->u.switch_.stream_location); - vkd3d_spirv_build_op_switch(builder, cf_info->u.switch_.selector_id, - cf_info->u.switch_.default_block_id, cf_info->u.switch_.case_blocks, - cf_info->u.switch_.case_block_count); - vkd3d_spirv_end_function_stream_insertion(builder); - - vkd3d_free(cf_info->u.switch_.case_blocks); - spirv_compiler_pop_control_flow_level(compiler); - break; - - case VKD3DSIH_CASE: - { - uint32_t label_id, value; - - assert(compiler->control_flow_depth); - assert(cf_info->current_block == VKD3D_BLOCK_SWITCH); - - if (src->swizzle != VKD3D_SHADER_SWIZZLE(X, X, X, X)) - { - WARN("Unexpected src swizzle %#x.\n", src->swizzle); - spirv_compiler_warning(compiler, VKD3D_SHADER_WARNING_SPV_INVALID_SWIZZLE, - "The swizzle for a switch case value is not scalar."); - } - assert(src->reg.type == VKD3DSPR_IMMCONST); - value = *src->reg.u.immconst_uint; - - if (!vkd3d_array_reserve((void **)&cf_info->u.switch_.case_blocks, &cf_info->u.switch_.case_blocks_size, - 2 * (cf_info->u.switch_.case_block_count + 1), sizeof(*cf_info->u.switch_.case_blocks))) - return VKD3D_ERROR_OUT_OF_MEMORY; - - label_id = vkd3d_spirv_alloc_id(builder); - if (cf_info->inside_block) /* fall-through */ - vkd3d_spirv_build_op_branch(builder, label_id); - - cf_info->u.switch_.case_blocks[2 * cf_info->u.switch_.case_block_count + 0] = value; - cf_info->u.switch_.case_blocks[2 * cf_info->u.switch_.case_block_count + 1] = label_id; - ++cf_info->u.switch_.case_block_count; - - vkd3d_spirv_build_op_label(builder, label_id); - cf_info->inside_block = true; - vkd3d_spirv_build_op_name(builder, label_id, "switch%u_case%u", cf_info->u.switch_.id, value); - break; - } - - case VKD3DSIH_DEFAULT: - assert(compiler->control_flow_depth); - assert(cf_info->current_block == VKD3D_BLOCK_SWITCH); - assert(!cf_info->u.switch_.default_block_id); - - cf_info->u.switch_.default_block_id = vkd3d_spirv_alloc_id(builder); - if (cf_info->inside_block) /* fall-through */ - vkd3d_spirv_build_op_branch(builder, cf_info->u.switch_.default_block_id); - - vkd3d_spirv_build_op_label(builder, cf_info->u.switch_.default_block_id); - vkd3d_spirv_build_op_name(builder, cf_info->u.switch_.default_block_id, - "switch%u_default", cf_info->u.switch_.id); - cf_info->inside_block = true; - break; - - case VKD3DSIH_BREAK: - { - struct vkd3d_control_flow_info *breakable_cf_info; - - assert(compiler->control_flow_depth); - - if (!(breakable_cf_info = spirv_compiler_find_innermost_breakable_cf_construct(compiler))) - { - FIXME("Unhandled break instruction.\n"); - return VKD3D_ERROR_INVALID_SHADER; - } - - if (breakable_cf_info->current_block == VKD3D_BLOCK_SWITCH) - { - /* The current case block may have already been ended by an - * unconditional continue instruction. */ - if (breakable_cf_info->inside_block) - vkd3d_spirv_build_op_branch(builder, breakable_cf_info->u.switch_.merge_block_id); - } - - cf_info->inside_block = false; - break; - } - - case VKD3DSIH_RET: - spirv_compiler_emit_return(compiler, instruction); - - if (cf_info) - cf_info->inside_block = false; - break; - - default: - ERR("Unexpected instruction %#x.\n", instruction->handler_idx); - break; - } - - return VKD3D_OK; -} - static bool spirv_compiler_init_blocks(struct spirv_compiler *compiler, unsigned int block_count) { compiler->block_count = block_count; @@ -7792,6 +7563,37 @@ static void spirv_compiler_emit_branch(struct spirv_compiler *compiler, spirv_compiler_get_label_id(compiler, src[1].reg.idx[1].offset)); } +static void spirv_compiler_emit_switch(struct spirv_compiler *compiler, + const struct vkd3d_shader_instruction *instruction) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + struct vkd3d_spirv_stream *stream = &builder->function_stream; + const struct vkd3d_shader_src_param *src = instruction->src; + uint32_t val_id; + size_t location; + unsigned int i; + + if (!vkd3d_swizzle_is_scalar(src[0].swizzle)) + { + WARN("Unexpected src swizzle %#x.\n", src[0].swizzle); + spirv_compiler_warning(compiler, VKD3D_SHADER_WARNING_SPV_INVALID_SWIZZLE, + "The swizzle for a switch value is not scalar."); + } + + val_id = spirv_compiler_emit_load_src(compiler, &src[0], VKD3DSP_WRITEMASK_0); + /* Emit the merge immediately before the switch instruction. */ + spirv_compiler_emit_merge(compiler, src[1].reg.idx[1].offset, 0); + location = vkd3d_spirv_stream_current_location(stream); + vkd3d_spirv_build_op_switch(builder, val_id, + spirv_compiler_get_label_id(compiler, src[1].reg.idx[0].offset), NULL, 0); + for (i = 2; i < instruction->src_count; ++i) + { + vkd3d_spirv_build_word(stream, src[i].reg.u.immconst_uint[0]); + vkd3d_spirv_build_word(stream, spirv_compiler_get_label_id(compiler, src[i].reg.idx[0].offset)); + } + vkd3d_spirv_stream_add_word_count(stream, location, (instruction->src_count - 2) * 2u); +} + static void spirv_compiler_emit_deriv_instruction(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { @@ -9545,19 +9347,8 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_F32TOF16: spirv_compiler_emit_f32tof16(compiler, instruction); break; - case VKD3DSIH_BREAK: - case VKD3DSIH_BREAKP: - case VKD3DSIH_CASE: - case VKD3DSIH_CONTINUE: - case VKD3DSIH_CONTINUEP: - case VKD3DSIH_DEFAULT: - case VKD3DSIH_ENDLOOP: - case VKD3DSIH_ENDSWITCH: - case VKD3DSIH_LABEL: - case VKD3DSIH_LOOP: case VKD3DSIH_RET: - case VKD3DSIH_SWITCH: - ret = spirv_compiler_emit_control_flow_instruction(compiler, instruction); + spirv_compiler_emit_return(compiler, instruction); break; case VKD3DSIH_RETP: spirv_compiler_emit_retc(compiler, instruction); @@ -9566,9 +9357,15 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_TEXKILL: spirv_compiler_emit_kill(compiler, instruction); break; + case VKD3DSIH_LABEL: + spirv_compiler_emit_label(compiler, instruction); + break; case VKD3DSIH_BRANCH: spirv_compiler_emit_branch(compiler, instruction); break; + case VKD3DSIH_SWITCH_MONOLITHIC: + spirv_compiler_emit_switch(compiler, instruction); + break; case VKD3DSIH_DSX: case VKD3DSIH_DSX_COARSE: case VKD3DSIH_DSX_FINE: diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index bda069503..6afc4640d 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -471,6 +471,7 @@ enum vkd3d_shader_opcode VKD3DSIH_SUB, VKD3DSIH_SWAPC, VKD3DSIH_SWITCH, + VKD3DSIH_SWITCH_MONOLITHIC, VKD3DSIH_SYNC, VKD3DSIH_TEX, VKD3DSIH_TEXBEM, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/dxbc.c | 5 +++ libs/vkd3d-shader/ir.c | 45 ++++++++++++++++++++++++ libs/vkd3d-shader/spirv.c | 8 +++++ libs/vkd3d-shader/vkd3d_shader_private.h | 3 ++ 4 files changed, 61 insertions(+) diff --git a/libs/vkd3d-shader/dxbc.c b/libs/vkd3d-shader/dxbc.c index 7834c1e16..de1463f1f 100644 --- a/libs/vkd3d-shader/dxbc.c +++ b/libs/vkd3d-shader/dxbc.c @@ -555,9 +555,14 @@ static int shdr_handler(const struct vkd3d_shader_dxbc_section_desc *section, void free_shader_desc(struct vkd3d_shader_desc *desc) { + size_t i; + shader_signature_cleanup(&desc->input_signature); shader_signature_cleanup(&desc->output_signature); shader_signature_cleanup(&desc->patch_constant_signature); + for (i = 0; i < desc->block_name_count; ++i) + vkd3d_free((void *)desc->block_names[i]); + vkd3d_free(desc->block_names); } int shader_extract_from_dxbc(const struct vkd3d_shader_code *dxbc, diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 0c3dad238..777b2eb4f 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1490,6 +1490,9 @@ struct cf_flattener size_t instruction_count; unsigned int block_id; + const char **block_names; + size_t block_name_capacity; + size_t block_name_count; unsigned int branch_id; unsigned int loop_id; @@ -1672,6 +1675,31 @@ static struct cf_flattener_info *cf_flattener_find_innermost_breakable_cf_constr return NULL; } +static void VKD3D_PRINTF_FUNC(3, 4) cf_flattener_create_block_name(struct cf_flattener *flattener, + unsigned int block_id, const char *fmt, ...) +{ + struct vkd3d_string_buffer buffer; + size_t block_name_count; + va_list args; + + --block_id; + + block_name_count = max(flattener->block_name_count, block_id + 1); + if (!vkd3d_array_reserve((void **)&flattener->block_names, &flattener->block_name_capacity, + block_name_count, sizeof(*flattener->block_names))) + return; + memset(&flattener->block_names[flattener->block_name_count], 0, + (block_name_count - flattener->block_name_count) * sizeof(*flattener->block_names)); + flattener->block_name_count = block_name_count; + + vkd3d_string_buffer_init(&buffer); + va_start(args, fmt); + vkd3d_string_buffer_vprintf(&buffer, fmt, args); + va_end(args); + + flattener->block_names[block_id] = buffer.buffer; +} + static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flattener *flattener) { struct vkd3d_shader_parser *parser = flattener->parser; @@ -1719,6 +1747,8 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte cf_info->inside_block = true; cf_info->current_block = VKD3D_BLOCK_IF; + cf_flattener_create_block_name(flattener, merge_block_id, "branch%u_merge", flattener->branch_id); + cf_flattener_create_block_name(flattener, true_block_id, "branch%u_true", flattener->branch_id); ++flattener->branch_id; break; @@ -1729,6 +1759,8 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte cf_info->u.if_.else_block_id = cf_flattener_alloc_block_id(flattener); cf_info->u.if_.targets->reg.idx[1].offset = cf_info->u.if_.else_block_id; + cf_flattener_create_block_name(flattener, + cf_info->u.if_.else_block_id, "branch%u_false", cf_info->u.if_.id); cf_flattener_emit_label(flattener, cf_info->u.if_.else_block_id); cf_info->inside_block = true; @@ -1765,6 +1797,10 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte cf_info->current_block = VKD3D_BLOCK_LOOP; cf_info->inside_block = true; + cf_flattener_create_block_name(flattener, loop_header_block_id, "loop%u_header", flattener->loop_id); + cf_flattener_create_block_name(flattener, loop_body_block_id, "loop%u_body", flattener->loop_id); + cf_flattener_create_block_name(flattener, continue_block_id, "loop%u_continue", flattener->loop_id); + cf_flattener_create_block_name(flattener, merge_block_id, "loop%u_merge", flattener->loop_id); ++flattener->loop_id; break; @@ -1802,6 +1838,8 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte cf_info->inside_block = false; cf_info->current_block = VKD3D_BLOCK_SWITCH; + cf_flattener_create_block_name(flattener, merge_block_id, "switch%u_merge", flattener->switch_id); + ++flattener->switch_id; if (!vkd3d_array_reserve((void **)&cf_info->u.switch_.cases, &cf_info->u.switch_.cases_size, @@ -1868,6 +1906,7 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte ++cf_info->u.switch_.cases_count; cf_flattener_emit_label(flattener, label_id); + cf_flattener_create_block_name(flattener, label_id, "switch%u_case%u", cf_info->u.switch_.id, value); cf_info->inside_block = true; break; } @@ -1878,6 +1917,9 @@ static enum vkd3d_result cf_flattener_iterate_instruction_array(struct cf_flatte cf_flattener_emit_unconditional_branch(flattener, cf_info->u.switch_.default_block_id); cf_flattener_emit_label(flattener, cf_info->u.switch_.default_block_id); + + cf_flattener_create_block_name(flattener, cf_info->u.switch_.default_block_id, + "switch%u_default", cf_info->u.switch_.id); cf_info->inside_block = true; break; @@ -2000,6 +2042,9 @@ static enum vkd3d_result flatten_control_flow_constructs(struct vkd3d_shader_par } vkd3d_free(flattener.control_flow_info); + /* Simpler to always free these in free_shader_desc(). */ + parser->shader_desc.block_names = flattener.block_names; + parser->shader_desc.block_name_count = flattener.block_name_count; return result; } diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 4a9d18db6..5f0da93d5 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2335,6 +2335,8 @@ struct spirv_compiler uint32_t *block_label_ids; unsigned int block_count; + const char **block_names; + size_t block_name_count; }; static bool is_in_default_phase(const struct spirv_compiler *compiler) @@ -7510,6 +7512,10 @@ static void spirv_compiler_emit_label(struct spirv_compiler *compiler, label_id = spirv_compiler_get_label_id(compiler, block_id); vkd3d_spirv_build_op_label(builder, label_id); + + --block_id; + if (block_id < compiler->block_name_count && compiler->block_names[block_id]) + vkd3d_spirv_build_op_name(builder, label_id, compiler->block_names[block_id]); } static void spirv_compiler_emit_merge(struct spirv_compiler *compiler, @@ -9570,6 +9576,8 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, memset(&shader_desc->output_signature, 0, sizeof(shader_desc->output_signature)); memset(&shader_desc->patch_constant_signature, 0, sizeof(shader_desc->patch_constant_signature)); compiler->use_vocp = parser->shader_desc.use_vocp; + compiler->block_names = parser->shader_desc.block_names; + compiler->block_name_count = parser->shader_desc.block_name_count; compiler->input_control_point_count = shader_desc->input_control_point_count; compiler->output_control_point_count = shader_desc->output_control_point_count; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 6afc4640d..5db00791b 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1015,6 +1015,9 @@ struct vkd3d_shader_desc } flat_constant_count[3]; bool use_vocp; + + const char **block_names; + size_t block_name_count; }; struct vkd3d_shader_register_semantic -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450
Incidentally, still not a fan of assert() in library code.
I had no idea. We've added quite a few lately. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450#note_55804
Incidentally, still not a fan of assert() in library code.
And I still don't understand why: they are very useful comments, they catch errors and if somebody does not like them it's easy to turn them off with `NDEBUG`, so the user ultimately has the choice to compromise between detecting problems early and keep going and hoping everything's fine. Removing assertions takes away advantages without giving any advantage that can't be easily obtained already with `NDEBUG`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450#note_55816
Incidentally, still not a fan of assert() in library code.
And I still don't understand why: they are very useful comments, they catch errors and if somebody does not like them it's easy to turn them off with `NDEBUG`, so the user ultimately has the choice to compromise between detecting problems early and keep going and hoping everything's fine. Removing assertions takes away advantages without giving any advantage that can't be easily obtained already with `NDEBUG`.
Fundamentally, because I don't think libraries should call abort(). Libraries only rarely know the full context they're running in; aborting other people's code is at best rude, and could e.g. have security implications or cause data loss in less benign cases. Moreover: - Aborting the application may be appropriate in some cases. If the reason the assert gets triggered is e.g. memory corruption, perhaps it makes sense to abort the application. Unfortunately we don't know that in advance; for something like e.g. https://bugs.winehq.org/show_bug.cgi?id=55190, I think aborting is worse that the issue being detected. - NDEBUG isn't quite as easy when you're using distribution packages. And if the issue we're detecting is indeed bad enough to justify aborting the application, NDEBUG doesn't make it better, it just hides the issue a little longer. - assert() isn't an error handling/reporting mechanism. vkd3d-shader should generally use vkd3d_shader_verror() for that. If we want vkd3d_shader_verror() to abort() for easier debugging, fine; add a local patch that does that, or introduce a VKD3D_SHADER_CONFIG flag. I don't think it should be the default behaviour. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450#note_55840
Fundamentally, because I don't think libraries should call abort(). Libraries only rarely know the full context they're running in; aborting other people's code is at best rude, and could e.g. have security implications or cause data loss in less benign cases.
It sounds like what you're objecting to is largely things like what FAudio does, i.e. assert() based on user input (cf. [1] for the assertion, [2] for the problems it caused). That absolutely is something we should avoid. I haven't fully checked whether that's the case in this commit, mind. However, the way I've used it, and the way I think it should be used, is not that; it's rather to document *and* double-check an internal assumption that, if violated, would basically lead to potentially any kind of error. The alternatives are (1) don't do that, which makes things a lot harder to debug if the error *does* happen, or (2) check and try to gracefully handle everything that would be an assert(), which I think quickly becomes untenable *and* unreadable. [I've seen code that actually does this and it wastes so much time and space.] The whole point of abort() is that, rude as it is, it's generally the least rude option. It's far safer and more polite than things like (a) pretending to succeed but giving incorrect output; (b) crashing a greater distance away from the root cause, e.g. inside the library user's code. [1] https://source.winehq.org/git/wine.git/blob/HEAD:/libs/faudio/src/F3DAudio.c... [2] https://bugs.winehq.org/show_bug.cgi?id=51921
Moreover:
* Aborting the application may be appropriate in some cases. If the reason the assert gets triggered is e.g. memory corruption, perhaps it makes sense to abort the application. Unfortunately we don't know that in advance; for something like e.g. https://bugs.winehq.org/show_bug.cgi?id=55190, I think aborting is worse that the issue being detected.
The tests don't make it clear, but if we just removed that assertion, we'd be outputting incorrect code. How is that better than aborting? Even if that example ended up being fine in practice, well, we wouldn't have known it in advance. assert() catches invariants that could result in *anything* if violated. I recognize I'm kind of arguing two opposite things here: one the one hand, we want to abort instead of doing something worse, but on the other hand, we don't want to have defense mechanisms because they take too much time and space. The appeal of assert() is that it is, I think, a happy medium. It catches real issues and makes debugging easier, but it's also cheap to write.
* NDEBUG isn't quite as easy when you're using distribution packages. And if the issue we're detecting is indeed bad enough to justify aborting the application, NDEBUG doesn't make it better, it just hides the issue a little longer.
Well, NDEBUG is another tradeoff. It's trading the performance you get from avoiding the 99% of successful asserts, for a potentially worse failure mode from the 1% of failed asserts. You fundamentally can't have both, so I don't see the tradeoff as a problem.
* assert() isn't an error handling/reporting mechanism. vkd3d-shader should generally use vkd3d_shader_verror() for that. If we want vkd3d_shader_verror() to abort() for easier debugging, fine; add a local patch that does that, or introduce a VKD3D_SHADER_CONFIG flag. I don't think it should be the default behaviour.
assert() as I'm using it is for catching *internal* assumptions. With all that said, should we use vkd3d_shader_error(VKD3D_SHADER_ERROR_ICE) instead of assert()? Probably? I think it's mildly objectively better, and about as cheap in most contexts. It doesn't have the NDEBUG tradeoff built in but we could add that easily enough. I just don't think assert() is awful either. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450#note_55845
How about we add an assert-like helper function which does nothing if not specifically enabled in the build. It can emit the source name and line number similar to `vkd3d_unreachable()`. We can use it internally on tests and shader collections. I've been adding asserts to document intended constraints and verify they hold broadly, but I think release builds shouldn't do that. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450#note_55867
```diff + case VKD3DSIH_BRANCH: + { + const struct vkd3d_shader_src_param *src = ins->src, *merge; + + if (ins->src_count > 2) + { + shader_dump_instruction_flags(compiler, ins); + vkd3d_string_buffer_printf(buffer, " "); + shader_dump_src_param(compiler, &src[0]); + shader_print_label_id(compiler, ", ", src[1].reg.idx[0].offset); + shader_print_label_id(compiler, ", ", src[1].reg.idx[1].offset); + merge = &src[2]; + } + else + { + shader_print_label_id(compiler, " ", src[0].reg.idx[0].offset); + merge = (ins->src_count > 1) ? &src[1] : NULL; + } + if (merge) + { + shader_print_label_id(compiler, ", ", merge->reg.idx[0].offset); + } + break; + } ```
Right, but if we used separate parameters for true/false and merge/continue, we could just let the default case handle this instruction. In principle that applies to VKD3DSIH_SWITCH_MONOLITHIC as well.
```diff +static unsigned int cf_flattener_alloc_block_id(struct cf_flattener *flattener) +{ + return ++flattener->block_id; +} ```
What happens when there are existing LABEL instructions in the source shader? These aren't terribly common in the wild, and the SPIR-V backend doesn't currently handle CALL/LABEL, but the instructions do exist in both d3dbc and tpf.
```diff +/* For conditional branches, this returns the true+false target branch parameter . */ +static struct vkd3d_shader_src_param *cf_flattener_emit_branch(struct cf_flattener *flattener, + unsigned int merge_block_id, const struct vkd3d_shader_src_param *condition, + unsigned int true_id, unsigned int false_id, + unsigned int flags) ```
Some stray space in the comment above.
```diff + case VKD3DSIH_IF: + if (!(cf_info = cf_flattener_push_control_flow_level(flattener))) + return VKD3D_ERROR_OUT_OF_MEMORY; + + true_block_id = cf_flattener_alloc_block_id(flattener); + merge_block_id = cf_flattener_alloc_block_id(flattener); + cf_info->u.if_.targets = cf_flattener_emit_branch(flattener, merge_block_id, + src, true_block_id, merge_block_id, instruction->flags); + if (!cf_info->u.if_.targets) + return VKD3D_ERROR_OUT_OF_MEMORY; + + cf_flattener_emit_label(flattener, true_block_id); + + cf_info->u.if_.id = flattener->branch_id; + cf_info->u.if_.merge_block_id = merge_block_id; + cf_info->u.if_.else_block_id = 0; + cf_info->inside_block = true; + cf_info->current_block = VKD3D_BLOCK_IF; + + ++flattener->branch_id; ```
Technically "branch_id" and "if_.id" aren't used until patch 7/7. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450#note_55907
Fundamentally, because I don't think libraries should call abort(). Libraries only rarely know the full context they're running in; aborting other people's code is at best rude, and could e.g. have security implications or cause data loss in less benign cases.
It sounds like what you're objecting to is largely things like what FAudio does, i.e. assert() based on user input (cf. \[1\] for the assertion, \[2\] for the problems it caused). That absolutely is something we should avoid.
Well, we've definitely had Wine bugs filed for assertions in the HLSL compiler triggered by trying to compile a particular shader.
I haven't fully checked whether that's the case in this commit, mind.
However, the way I've used it, and the way I think it should be used, is not that; it's rather to document *and* double-check an internal assumption that, if violated, would basically lead to potentially any kind of error.
The alternatives are (1) don't do that, which makes things a lot harder to debug if the error *does* happen, or (2) check and try to gracefully handle everything that would be an assert(), which I think quickly becomes untenable *and* unreadable. \[I've seen code that actually does this and it wastes so much time and space.\]
The whole point of abort() is that, rude as it is, it's generally the least rude option. It's far safer and more polite than things like (a) pretending to succeed but giving incorrect output; (b) crashing a greater distance away from the root cause, e.g. inside the library user's code.
I don't think that's a decision a library can reliably make.
Moreover:
* Aborting the application may be appropriate in some cases. If the reason the assert gets triggered is e.g. memory corruption, perhaps it makes sense to abort the application. Unfortunately we don't know that in advance; for something like e.g. https://bugs.winehq.org/show_bug.cgi?id=55190, I think aborting is worse that the issue being detected.
The tests don't make it clear, but if we just removed that assertion, we'd be outputting incorrect code. How is that better than aborting?
It mostly depends on the context we're running in. If we're being fed malicious input, returning incorrect output may very well be the better option. But those aren't the only two options we have, of course. For the cases where incorrect output would be the worst that could happen, I think the right answer is pretty much vkd3d_shader_error().
Even if that example ended up being fine in practice, well, we wouldn't have known it in advance. assert() catches invariants that could result in *anything* if violated.
In theory, sure. In practice, I think we have a number of cases where assert() is essentially used as a substitute for input validation. It also tends to make review a bit harder; instead of verifying the condition is handled appropriately, the reviewer needs to verify it can never happen.
* assert() isn't an error handling/reporting mechanism. vkd3d-shader should generally use vkd3d_shader_verror() for that. If we want vkd3d_shader_verror() to abort() for easier debugging, fine; add a local patch that does that, or introduce a VKD3D_SHADER_CONFIG flag. I don't think it should be the default behaviour.
assert() as I'm using it is for catching *internal* assumptions.
With all that said, should we use vkd3d_shader_error(VKD3D_SHADER_ERROR_ICE) instead of assert()? Probably? I think it's mildly objectively better, and about as cheap in most contexts. It doesn't have the NDEBUG tradeoff built in but we could add that easily enough.
I just don't think assert() is awful either.
I think I've been fairly lenient about accepting these in the HLSL compiler in particular, if grudgingly. Still, I'd rather see fewer assert()'s than more. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/450#note_55918
participants (5)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)