[PATCH v5 0/5] MR749: vkd3d-shader/ir: Minimise temp usage.
Overall this is simpler, results in less verbose SPIR-V code which is easier to debug, and eliminates stuttering issues in Horizon Zero Dawn when run in an SM 6 dev branch with Gio's cfg4 structuriser branch. -- v5: vkd3d-shader/ir: Materialise SSAs to temps before lowering switch instructions. vkd3d-shader/ir: Convert SSAs to temps only if the block of origin does not dominate all uses. vkd3d-shader/spirv: Handle uint2 to double bitcast in spirv_compiler_emit_mov(). vkd3d-shader/spirv: Emit a uint result for RESINFO_UINT if the dst register is SSA. vkd3d-shader/ir: Materialise phis to temps in the incoming blocks. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749
From: Conor McCarthy <cmccarthy(a)codeweavers.com> RADV converts temps to phi instructions, so converting phis to MOVC in VSIR just translates back to phis feeding into a MOVC. This commit eliminates the MOVC. --- libs/vkd3d-shader/ir.c | 195 +++++++++++++---------------------------- 1 file changed, 60 insertions(+), 135 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 7230d0e8b..8c6a5c3fc 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -2717,109 +2717,80 @@ static void materialize_ssas_to_temps_process_src_param(struct vsir_program *pro materialize_ssas_to_temps_process_reg(program, &src->reg); } -static const struct vkd3d_shader_src_param *materialize_ssas_to_temps_compute_source(struct vkd3d_shader_instruction *ins, - unsigned int label) +struct ssas_to_temps_block_info { - unsigned int i; - - assert(ins->handler_idx == VKD3DSIH_PHI); - - for (i = 0; i < ins->src_count; i += 2) + struct phi_incoming_to_temp { - if (label_from_src_param(&ins->src[i + 1]) == label) - return &ins->src[i]; - } - - vkd3d_unreachable(); -} + struct vkd3d_shader_src_param *src; + struct vkd3d_shader_dst_param *dst; + } *incomings; + size_t incoming_capacity; + size_t incoming_count; +}; -static bool materialize_ssas_to_temps_synthesize_mov(struct vsir_program *program, - struct vkd3d_shader_instruction *instruction, const struct vkd3d_shader_location *loc, - const struct vkd3d_shader_dst_param *dest, const struct vkd3d_shader_src_param *cond, - const struct vkd3d_shader_src_param *source, bool invert) +static void ssas_to_temps_block_info_cleanup(struct ssas_to_temps_block_info *block_info, + size_t count) { - struct vkd3d_shader_src_param *src; - struct vkd3d_shader_dst_param *dst; - - if (!vsir_instruction_init_with_params(program, instruction, loc, - cond ? VKD3DSIH_MOVC : VKD3DSIH_MOV, 1, cond ? 3 : 1)) - return false; - - dst = instruction->dst; - src = instruction->src; - - dst[0] = *dest; - materialize_ssas_to_temps_process_dst_param(program, &dst[0]); - - assert(dst[0].write_mask == VKD3DSP_WRITEMASK_0); - assert(dst[0].modifiers == 0); - assert(dst[0].shift == 0); + size_t i; - if (cond) - { - src[0] = *cond; - src[1 + invert] = *source; - memset(&src[2 - invert], 0, sizeof(src[2 - invert])); - src[2 - invert].reg = dst[0].reg; - materialize_ssas_to_temps_process_src_param(program, &src[1]); - materialize_ssas_to_temps_process_src_param(program, &src[2]); - } - else - { - src[0] = *source; - materialize_ssas_to_temps_process_src_param(program, &src[0]); - } + for (i = 0; i < count; ++i) + vkd3d_free(block_info[i].incomings); - return true; + vkd3d_free(block_info); } static enum vkd3d_result vsir_program_materialise_ssas_to_temps(struct vsir_program *program) { + size_t ins_capacity = 0, ins_count = 0, phi_count, incoming_count, i; + struct ssas_to_temps_block_info *info, *block_info = NULL; struct vkd3d_shader_instruction *instructions = NULL; - struct materialize_ssas_to_temps_block_data - { - size_t phi_begin; - size_t phi_count; - } *block_index = NULL; - size_t ins_capacity = 0, ins_count = 0, i; unsigned int current_label = 0; - if (!reserve_instructions(&instructions, &ins_capacity, program->instructions.count)) - goto fail; - - if (!(block_index = vkd3d_calloc(program->block_count, sizeof(*block_index)))) + if (!(block_info = vkd3d_calloc(program->block_count, sizeof(*block_info)))) { - ERR("Failed to allocate block index.\n"); + ERR("Failed to allocate block info array.\n"); goto fail; } - for (i = 0; i < program->instructions.count; ++i) + for (i = 0, phi_count = 0, incoming_count = 0; i < program->instructions.count; ++i) { struct vkd3d_shader_instruction *ins = &program->instructions.elements[i]; + unsigned int j; - switch (ins->handler_idx) + if (ins->handler_idx != VKD3DSIH_PHI) + continue; + ++phi_count; + + for (j = 0; j < ins->src_count; j += 2) { - case VKD3DSIH_LABEL: - current_label = label_from_src_param(&ins->src[0]); - break; + struct phi_incoming_to_temp *incoming; + unsigned int label; - case VKD3DSIH_PHI: - assert(current_label != 0); - assert(i != 0); - if (block_index[current_label - 1].phi_begin == 0) - block_index[current_label - 1].phi_begin = i; - block_index[current_label - 1].phi_count += 1; - break; + label = label_from_src_param(&ins->src[j + 1]); + assert(label); - default: - current_label = 0; - break; + info = &block_info[label - 1]; + + if (!(vkd3d_array_reserve((void **)&info->incomings, &info->incoming_capacity, info->incoming_count + 1, + sizeof(*info->incomings)))) + goto fail; + + incoming = &info->incomings[info->incoming_count++]; + incoming->src = &ins->src[j]; + incoming->dst = ins->dst; + + ++incoming_count; } + + materialize_ssas_to_temps_process_dst_param(program, ins->dst); } + if (!reserve_instructions(&instructions, &ins_capacity, program->instructions.count + incoming_count - phi_count)) + goto fail; + for (i = 0; i < program->instructions.count; ++i) { - struct vkd3d_shader_instruction *ins = &program->instructions.elements[i]; + struct vkd3d_shader_instruction *mov_ins, *ins = &program->instructions.elements[i]; size_t j; for (j = 0; j < ins->dst_count; ++j) @@ -2835,65 +2806,21 @@ static enum vkd3d_result vsir_program_materialise_ssas_to_temps(struct vsir_prog break; case VKD3DSIH_BRANCH: - { - if (vsir_register_is_label(&ins->src[0].reg)) - { - const struct materialize_ssas_to_temps_block_data *data = &block_index[label_from_src_param(&ins->src[0]) - 1]; - - if (!reserve_instructions(&instructions, &ins_capacity, ins_count + data->phi_count)) - goto fail; - - for (j = data->phi_begin; j < data->phi_begin + data->phi_count; ++j) - { - const struct vkd3d_shader_src_param *source; - - source = materialize_ssas_to_temps_compute_source(&program->instructions.elements[j], - current_label); - if (!materialize_ssas_to_temps_synthesize_mov(program, &instructions[ins_count], - &ins->location, &program->instructions.elements[j].dst[0], NULL, source, false)) - goto fail; + case VKD3DSIH_RET: + info = &block_info[current_label - 1]; - ++ins_count; - } - } - else + for (j = 0; j < info->incoming_count; ++j) { - struct materialize_ssas_to_temps_block_data *data_true = &block_index[label_from_src_param(&ins->src[1]) - 1], - *data_false = &block_index[label_from_src_param(&ins->src[2]) - 1]; - const struct vkd3d_shader_src_param *cond = &ins->src[0]; - - if (!reserve_instructions(&instructions, &ins_capacity, - ins_count + data_true->phi_count + data_false->phi_count)) - goto fail; - - for (j = data_true->phi_begin; j < data_true->phi_begin + data_true->phi_count; ++j) - { - const struct vkd3d_shader_src_param *source; - - source = materialize_ssas_to_temps_compute_source(&program->instructions.elements[j], - current_label); - if (!materialize_ssas_to_temps_synthesize_mov(program, &instructions[ins_count], - &ins->location, &program->instructions.elements[j].dst[0], cond, source, false)) - goto fail; - - ++ins_count; - } - - for (j = data_false->phi_begin; j < data_false->phi_begin + data_false->phi_count; ++j) - { - const struct vkd3d_shader_src_param *source; - - source = materialize_ssas_to_temps_compute_source(&program->instructions.elements[j], - current_label); - if (!materialize_ssas_to_temps_synthesize_mov(program, &instructions[ins_count], - &ins->location, &program->instructions.elements[j].dst[0], cond, source, true)) - goto fail; - - ++ins_count; - } + struct phi_incoming_to_temp *incoming = &info->incomings[j]; + + mov_ins = &instructions[ins_count++]; + vsir_instruction_init(mov_ins, &ins->location, VKD3DSIH_MOV); + mov_ins->src = incoming->src; + mov_ins->src_count = 1; + mov_ins->dst = incoming->dst; + mov_ins->dst_count = 1; } break; - } case VKD3DSIH_PHI: continue; @@ -2902,25 +2829,23 @@ static enum vkd3d_result vsir_program_materialise_ssas_to_temps(struct vsir_prog break; } - if (!reserve_instructions(&instructions, &ins_capacity, ins_count + 1)) - goto fail; - instructions[ins_count++] = *ins; } vkd3d_free(program->instructions.elements); - vkd3d_free(block_index); program->instructions.elements = instructions; program->instructions.capacity = ins_capacity; program->instructions.count = ins_count; program->temp_count += program->ssa_count; program->ssa_count = 0; + ssas_to_temps_block_info_cleanup(block_info, program->block_count); + return VKD3D_OK; fail: vkd3d_free(instructions); - vkd3d_free(block_index); + ssas_to_temps_block_info_cleanup(block_info, program->block_count); return VKD3D_ERROR_OUT_OF_MEMORY; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 46130244c..b70bc30e7 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -9334,6 +9334,7 @@ static void spirv_compiler_emit_resinfo(struct spirv_compiler *compiler, const struct vkd3d_shader_dst_param *dst = instruction->dst; const struct vkd3d_shader_src_param *src = instruction->src; uint32_t type_id, lod_id, val_id, miplevel_count_id; + enum vkd3d_shader_component_type component_type; uint32_t constituents[VKD3D_VEC4_SIZE]; unsigned int i, size_component_count; struct vkd3d_shader_image image; @@ -9370,10 +9371,16 @@ static void spirv_compiler_emit_resinfo(struct spirv_compiler *compiler, val_id = vkd3d_spirv_build_op_composite_construct(builder, type_id, constituents, i + 2); + component_type = VKD3D_SHADER_COMPONENT_FLOAT; + type_id = vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_FLOAT, VKD3D_VEC4_SIZE); if (instruction->flags == VKD3DSI_RESINFO_UINT) { - val_id = vkd3d_spirv_build_op_bitcast(builder, type_id, val_id); + /* SSA registers must match the specified result type. */ + if (!register_is_ssa(&dst->reg)) + val_id = vkd3d_spirv_build_op_bitcast(builder, type_id, val_id); + else + component_type = VKD3D_SHADER_COMPONENT_UINT; } else { @@ -9382,7 +9389,7 @@ static void spirv_compiler_emit_resinfo(struct spirv_compiler *compiler, val_id = vkd3d_spirv_build_op_convert_utof(builder, type_id, val_id); } val_id = spirv_compiler_emit_swizzle(compiler, val_id, VKD3DSP_WRITEMASK_ALL, - VKD3D_SHADER_COMPONENT_FLOAT, src[1].swizzle, dst->write_mask); + component_type, src[1].swizzle, dst->write_mask); spirv_compiler_emit_store_dst(compiler, dst, val_id); } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Necessary for MakeDouble if the dst is SSA. --- libs/vkd3d-shader/spirv.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index b70bc30e7..78c95bde8 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -7283,8 +7283,12 @@ static void spirv_compiler_emit_mov(struct spirv_compiler *compiler, } general_implementation: - write_mask = (src->reg.type == VKD3DSPR_IMMCONST64 && !data_type_is_64_bit(dst->reg.data_type)) - ? vsir_write_mask_64_from_32(dst->write_mask) : dst->write_mask; + write_mask = dst->write_mask; + if (src->reg.type == VKD3DSPR_IMMCONST64 && !data_type_is_64_bit(dst->reg.data_type)) + write_mask = vsir_write_mask_64_from_32(write_mask); + else if (!data_type_is_64_bit(src->reg.data_type) && data_type_is_64_bit(dst->reg.data_type)) + write_mask = vsir_write_mask_32_from_64(write_mask); + val_id = spirv_compiler_emit_load_src(compiler, src, write_mask); if (dst->reg.data_type != src->reg.data_type) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 185 ++++++++++++++++++++++++++++++++++------- 1 file changed, 157 insertions(+), 28 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 8c6a5c3fc..6bc6add1d 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -2685,36 +2685,43 @@ fail: return VKD3D_ERROR_OUT_OF_MEMORY; } -static void materialize_ssas_to_temps_process_src_param(struct vsir_program *program, - struct vkd3d_shader_src_param *src); +struct ssas_to_temps_alloc +{ + unsigned int *table; + unsigned int next_temp_idx; +}; + +static bool ssas_to_temps_alloc_init(struct ssas_to_temps_alloc *alloc, unsigned int ssa_count, unsigned int temp_count) +{ + size_t i = ssa_count * sizeof(*alloc->table); + + if (!(alloc->table = vkd3d_malloc(i))) + { + ERR("Failed to allocate SSA table.\n"); + return false; + } + memset(alloc->table, 0xff, i); + + alloc->next_temp_idx = temp_count; + return true; +} /* This is idempotent: it can be safely applied more than once on the * same register. */ -static void materialize_ssas_to_temps_process_reg(struct vsir_program *program, struct vkd3d_shader_register *reg) +static void materialize_ssas_to_temps_process_reg(struct vsir_program *program, struct ssas_to_temps_alloc *alloc, + struct vkd3d_shader_register *reg) { unsigned int i; - if (reg->type == VKD3DSPR_SSA) + if (reg->type == VKD3DSPR_SSA && alloc->table[reg->idx[0].offset] != UINT_MAX) { reg->type = VKD3DSPR_TEMP; - reg->idx[0].offset += program->temp_count; + reg->idx[0].offset = alloc->table[reg->idx[0].offset]; } for (i = 0; i < reg->idx_count; ++i) if (reg->idx[i].rel_addr) - materialize_ssas_to_temps_process_src_param(program, reg->idx[i].rel_addr); -} - -static void materialize_ssas_to_temps_process_dst_param(struct vsir_program *program, - struct vkd3d_shader_dst_param *dst) -{ - materialize_ssas_to_temps_process_reg(program, &dst->reg); -} - -static void materialize_ssas_to_temps_process_src_param(struct vsir_program *program, - struct vkd3d_shader_src_param *src) -{ - materialize_ssas_to_temps_process_reg(program, &src->reg); + materialize_ssas_to_temps_process_reg(program, alloc, ®->idx[i].rel_addr->reg); } struct ssas_to_temps_block_info @@ -2723,6 +2730,7 @@ struct ssas_to_temps_block_info { struct vkd3d_shader_src_param *src; struct vkd3d_shader_dst_param *dst; + unsigned int temp_idx; } *incomings; size_t incoming_capacity; size_t incoming_count; @@ -2744,6 +2752,7 @@ static enum vkd3d_result vsir_program_materialise_ssas_to_temps(struct vsir_prog size_t ins_capacity = 0, ins_count = 0, phi_count, incoming_count, i; struct ssas_to_temps_block_info *info, *block_info = NULL; struct vkd3d_shader_instruction *instructions = NULL; + struct ssas_to_temps_alloc alloc = {0}; unsigned int current_label = 0; if (!(block_info = vkd3d_calloc(program->block_count, sizeof(*block_info)))) @@ -2752,15 +2761,22 @@ static enum vkd3d_result vsir_program_materialise_ssas_to_temps(struct vsir_prog goto fail; } + if (!ssas_to_temps_alloc_init(&alloc, program->ssa_count, program->temp_count)) + goto fail; + for (i = 0, phi_count = 0, incoming_count = 0; i < program->instructions.count; ++i) { struct vkd3d_shader_instruction *ins = &program->instructions.elements[i]; - unsigned int j; + unsigned int j, temp_idx; + /* Only phi src/dst SSA values need be converted here. Structurisation may + * introduce new cases of undominated SSA use, which will be handled later. */ if (ins->handler_idx != VKD3DSIH_PHI) continue; ++phi_count; + temp_idx = alloc.next_temp_idx++; + for (j = 0; j < ins->src_count; j += 2) { struct phi_incoming_to_temp *incoming; @@ -2778,13 +2794,19 @@ static enum vkd3d_result vsir_program_materialise_ssas_to_temps(struct vsir_prog incoming = &info->incomings[info->incoming_count++]; incoming->src = &ins->src[j]; incoming->dst = ins->dst; + incoming->temp_idx = temp_idx; + + alloc.table[ins->dst->reg.idx[0].offset] = temp_idx; ++incoming_count; } - materialize_ssas_to_temps_process_dst_param(program, ins->dst); + materialize_ssas_to_temps_process_reg(program, &alloc, &ins->dst->reg); } + if (!phi_count) + goto done; + if (!reserve_instructions(&instructions, &ins_capacity, program->instructions.count + incoming_count - phi_count)) goto fail; @@ -2794,10 +2816,10 @@ static enum vkd3d_result vsir_program_materialise_ssas_to_temps(struct vsir_prog size_t j; for (j = 0; j < ins->dst_count; ++j) - materialize_ssas_to_temps_process_dst_param(program, &ins->dst[j]); + materialize_ssas_to_temps_process_reg(program, &alloc, &ins->dst[j].reg); for (j = 0; j < ins->src_count; ++j) - materialize_ssas_to_temps_process_src_param(program, &ins->src[j]); + materialize_ssas_to_temps_process_reg(program, &alloc, &ins->src[j].reg); switch (ins->handler_idx) { @@ -2836,16 +2858,17 @@ static enum vkd3d_result vsir_program_materialise_ssas_to_temps(struct vsir_prog program->instructions.elements = instructions; program->instructions.capacity = ins_capacity; program->instructions.count = ins_count; - program->temp_count += program->ssa_count; - program->ssa_count = 0; - + program->temp_count = alloc.next_temp_idx; +done: ssas_to_temps_block_info_cleanup(block_info, program->block_count); + vkd3d_free(alloc.table); return VKD3D_OK; fail: vkd3d_free(instructions); ssas_to_temps_block_info_cleanup(block_info, program->block_count); + vkd3d_free(alloc.table); return VKD3D_ERROR_OUT_OF_MEMORY; } @@ -4310,6 +4333,98 @@ fail: return ret; } +static void register_map_undominated_use(struct vkd3d_shader_register *reg, struct ssas_to_temps_alloc *alloc, + struct vsir_block *block, struct vsir_block **origin_blocks) +{ + unsigned int i; + + if (!register_is_ssa(reg)) + return; + + i = reg->idx[0].offset; + if (alloc->table[i] == UINT_MAX && !vsir_block_dominates(origin_blocks[i], block)) + alloc->table[i] = alloc->next_temp_idx++; + + for (i = 0; i < reg->idx_count; ++i) + if (reg->idx[i].rel_addr) + register_map_undominated_use(®->idx[i].rel_addr->reg, alloc, block, origin_blocks); +} + +/* Drivers are not necessarily optimised to handle very large numbers of temps. For example, + * using them only where necessary fixes stuttering issues in Horizon Zero Dawn on RADV. + * This can also result in the backend emitting less code because temps typically need an + * access chain and a load/store. Conversion of phi SSA values to temps should eliminate all + * undominated SSA use, but structurisation may create new occurrences. */ +static enum vkd3d_result vsir_cfg_materialize_undominated_ssas_to_temps(struct vsir_cfg *cfg) +{ + struct vsir_program *program = cfg->program; + struct ssas_to_temps_alloc alloc = {0}; + struct vsir_block **origin_blocks; + unsigned int j; + size_t i; + + if (!(origin_blocks = vkd3d_calloc(program->ssa_count, sizeof(*origin_blocks)))) + { + ERR("Failed to allocate origin block array.\n"); + return VKD3D_ERROR_OUT_OF_MEMORY; + } + if (!ssas_to_temps_alloc_init(&alloc, program->ssa_count, program->temp_count)) + { + vkd3d_free(origin_blocks); + return VKD3D_ERROR_OUT_OF_MEMORY; + } + + for (i = 0; i < cfg->block_count; ++i) + { + struct vsir_block *block = &cfg->blocks[i]; + struct vkd3d_shader_instruction *ins; + + for (ins = block->begin; ins <= block->end; ++ins) + { + for (j = 0; j < ins->dst_count; ++j) + { + if (register_is_ssa(&ins->dst[j].reg)) + origin_blocks[ins->dst[j].reg.idx[0].offset] = block; + } + } + } + + for (i = 0; i < cfg->block_count; ++i) + { + struct vsir_block *block = &cfg->blocks[i]; + struct vkd3d_shader_instruction *ins; + + for (ins = block->begin; ins <= block->end; ++ins) + { + for (j = 0; j < ins->src_count; ++j) + register_map_undominated_use(&ins->src[j].reg, &alloc, block, origin_blocks); + } + } + + if (alloc.next_temp_idx == program->temp_count) + goto done; + + TRACE("Emitting temps for %u values with undominated usage.\n", alloc.next_temp_idx - program->temp_count); + + for (i = 0; i < program->instructions.count; ++i) + { + struct vkd3d_shader_instruction *ins = &program->instructions.elements[i]; + + for (j = 0; j < ins->dst_count; ++j) + materialize_ssas_to_temps_process_reg(program, &alloc, &ins->dst[j].reg); + + for (j = 0; j < ins->src_count; ++j) + materialize_ssas_to_temps_process_reg(program, &alloc, &ins->src[j].reg); + } + + program->temp_count = alloc.next_temp_idx; +done: + vkd3d_free(origin_blocks); + vkd3d_free(alloc.table); + + return VKD3D_OK; +} + struct validation_context { struct vkd3d_shader_message_context *message_context; @@ -5231,6 +5346,20 @@ enum vkd3d_result vsir_program_normalise(struct vsir_program *program, uint64_t } vsir_cfg_cleanup(&cfg); + + if ((result = vsir_program_flatten_control_flow_constructs(program, message_context)) < 0) + return result; + + if ((result = vsir_cfg_init(&cfg, program, message_context)) < 0) + return result; + vsir_cfg_compute_dominators(&cfg); + + result = vsir_cfg_materialize_undominated_ssas_to_temps(&cfg); + + vsir_cfg_cleanup(&cfg); + + if (result < 0) + return result; } else { @@ -5260,10 +5389,10 @@ enum vkd3d_result vsir_program_normalise(struct vsir_program *program, uint64_t if ((result = vsir_program_normalise_combined_samplers(program, message_context)) < 0) return result; - } - if ((result = vsir_program_flatten_control_flow_constructs(program, message_context)) < 0) - return result; + if ((result = vsir_program_flatten_control_flow_constructs(program, message_context)) < 0) + return result; + } if (TRACE_ON()) vkd3d_shader_trace(program); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 96 ++---------------------------------------- 1 file changed, 3 insertions(+), 93 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 6bc6add1d..05f3c34d1 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -2577,97 +2577,6 @@ static enum vkd3d_result lower_switch_to_if_ladder(struct vsir_program *program) } } - /* Second subpass: creating new blocks might have broken - * references in PHI instructions, so we use the block map to fix - * them. */ - current_label = 0; - for (i = 0; i < ins_count; ++i) - { - struct vkd3d_shader_instruction *ins = &instructions[i]; - struct vkd3d_shader_src_param *new_src; - unsigned int j, l, new_src_count = 0; - - switch (ins->handler_idx) - { - case VKD3DSIH_LABEL: - current_label = label_from_src_param(&ins->src[0]); - continue; - - case VKD3DSIH_PHI: - break; - - default: - continue; - } - - /* First count how many source parameters we need. */ - for (j = 0; j < ins->src_count; j += 2) - { - unsigned int source_label = label_from_src_param(&ins->src[j + 1]); - size_t k, match_count = 0; - - for (k = 0; k < map_count; ++k) - { - struct lower_switch_to_if_ladder_block_mapping *mapping = &block_map[k]; - - if (mapping->switch_label == source_label && mapping->target_label == current_label) - match_count += 1; - } - - new_src_count += (match_count != 0) ? 2 * match_count : 2; - } - - assert(new_src_count >= ins->src_count); - - /* Allocate more source parameters if needed. */ - if (new_src_count == ins->src_count) - { - new_src = ins->src; - } - else - { - if (!(new_src = vsir_program_get_src_params(program, new_src_count))) - { - ERR("Failed to allocate %u source parameters.\n", new_src_count); - goto fail; - } - } - - /* Then do the copy. */ - for (j = 0, l = 0; j < ins->src_count; j += 2) - { - unsigned int source_label = label_from_src_param(&ins->src[j + 1]); - size_t k, match_count = 0; - - for (k = 0; k < map_count; ++k) - { - struct lower_switch_to_if_ladder_block_mapping *mapping = &block_map[k]; - - if (mapping->switch_label == source_label && mapping->target_label == current_label) - { - match_count += 1; - - new_src[l] = ins->src[j]; - new_src[l + 1] = ins->src[j + 1]; - new_src[l + 1].reg.idx[0].offset = mapping->if_label; - l += 2; - } - } - - if (match_count == 0) - { - new_src[l] = ins->src[j]; - new_src[l + 1] = ins->src[j + 1]; - l += 2; - } - } - - assert(l == new_src_count); - - ins->src_count = new_src_count; - ins->src = new_src; - } - vkd3d_free(program->instructions.elements); vkd3d_free(block_map); program->instructions.elements = instructions; @@ -2829,6 +2738,7 @@ static enum vkd3d_result vsir_program_materialise_ssas_to_temps(struct vsir_prog case VKD3DSIH_BRANCH: case VKD3DSIH_RET: + case VKD3DSIH_SWITCH_MONOLITHIC: info = &block_info[current_label - 1]; for (j = 0; j < info->incoming_count; ++j) @@ -5304,10 +5214,10 @@ enum vkd3d_result vsir_program_normalise(struct vsir_program *program, uint64_t { struct vsir_cfg cfg; - if ((result = lower_switch_to_if_ladder(program)) < 0) + if ((result = vsir_program_materialise_ssas_to_temps(program)) < 0) return result; - if ((result = vsir_program_materialise_ssas_to_temps(program)) < 0) + if ((result = lower_switch_to_if_ladder(program)) < 0) return result; if ((result = vsir_cfg_init(&cfg, program, message_context)) < 0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/ir.c:
- { - const struct materialize_ssas_to_temps_block_data *data = &block_index[label_from_src_param(&ins->src[0]) - 1]; - - if (!reserve_instructions(&instructions, &ins_capacity, ins_count + data->phi_count)) - goto fail; - - for (j = data->phi_begin; j < data->phi_begin + data->phi_count; ++j) - { - const struct vkd3d_shader_src_param *source; - - source = materialize_ssas_to_temps_compute_source(&program->instructions.elements[j], - current_label); - if (!materialize_ssas_to_temps_synthesize_mov(program, &instructions[ins_count], - &ins->location, &program->instructions.elements[j].dst[0], NULL, source, false)) - goto fail; + case VKD3DSIH_RET: Technically it doesn't look wrong, but is there a reason for this? If we're returning there should be no incoming here.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749#note_66913
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/ir.c:
- source = materialize_ssas_to_temps_compute_source(&program->instructions.elements[j], - current_label); - if (!materialize_ssas_to_temps_synthesize_mov(program, &instructions[ins_count], - &ins->location, &program->instructions.elements[j].dst[0], cond, source, true)) - goto fail; - - ++ins_count; - } + struct phi_incoming_to_temp *incoming = &info->incomings[j]; + + mov_ins = &instructions[ins_count++]; + vsir_instruction_init(mov_ins, &ins->location, VKD3DSIH_MOV); + mov_ins->src = incoming->src; + mov_ins->src_count = 1; + mov_ins->dst = incoming->dst; + mov_ins->dst_count = 1; Hmm, I'm not sure I like this: unless I'm missing something, it means that might end up with many instructions pointing to the same `struct vkd3d_shader_register` object, which might be confusing for some later pass if, say, they write to a register and some other seemingly unrelated register is altered too. I'd suggest to rather allocate a new register and copy the data. It probably uses some more memory, but we might be saving ourselves a lot of debugging headache in the future.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749#note_66914
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/ir.c:
- current_label = 0; - break; + info = &block_info[label - 1]; + + if (!(vkd3d_array_reserve((void **)&info->incomings, &info->incoming_capacity, info->incoming_count + 1, + sizeof(*info->incomings)))) + goto fail; + + incoming = &info->incomings[info->incoming_count++]; + incoming->src = &ins->src[j]; + incoming->dst = ins->dst; + + ++incoming_count; } + + materialize_ssas_to_temps_process_dst_param(program, ins->dst); Shouldn't you process the sources too? The PHI instruction is soon going to be dropped, but the parameters that are referenced by `incoming` will be later inserted back in the code stream, and the sources might very well come from another PHI instruction, I guess.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749#note_66915
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/ir.c:
goto fail; }
+ if (!ssas_to_temps_alloc_init(&alloc, program->ssa_count, program->temp_count)) + goto fail; + for (i = 0, phi_count = 0, incoming_count = 0; i < program->instructions.count; ++i) { struct vkd3d_shader_instruction *ins = &program->instructions.elements[i]; - unsigned int j; + unsigned int j, temp_idx;
+ /* Only phi src/dst SSA values need be converted here. Structurisation may + * introduce new cases of undominated SSA use, which will be handled later. */
That makes sense, but then we should also rename the pass, otherwise its name becomes misleading. Something like `vsir_program_materialize_phi_ssas_to_temps()`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749#note_66916
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/ir.c:
{ struct vkd3d_shader_src_param *src; struct vkd3d_shader_dst_param *dst; + unsigned int temp_idx;
It seems this is assigned but never used. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749#note_66917
On Thu Apr 4 12:30:35 2024 +0000, Giovanni Mascellani wrote:
Shouldn't you process the sources too? The PHI instruction is soon going to be dropped, but the parameters that are referenced by `incoming` will be later inserted back in the code stream, and the sources might very well come from another PHI instruction, I guess. If sources are from another phi, they will be processed as a dst by this call on another iteration.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749#note_66933
On Thu Apr 4 12:30:34 2024 +0000, Giovanni Mascellani wrote:
Hmm, I'm not sure I like this: unless I'm missing something, it means that might end up with many instructions pointing to the same `struct vkd3d_shader_register` object, which might be confusing for some later pass if, say, they write to a register and some other seemingly unrelated register is altered too. I'd suggest to rather allocate a new register and copy the data. It probably uses some more memory, but we might be saving ourselves a lot of debugging headache in the future. Maybe, but I think the constraints are sufficient here. If one dst is modified somewhere, they should all be, since they represent a single phi result. In other words, assuming they are unrelated would itself be an error.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/749#note_66935
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)