[PATCH v2 0/3] MR487: vkd3d-shader/hlsl: Fix infinite loops when applying copy-prop to swizzles, version 3.
Supersedes !482. Instead of only storing the value that each variable's component has at the moment of the instruction currently handled by copy-prop, we store the trace of all the historic values with their timestamps, i.e. the instruction index on which the value was stored. This allow us to query the value that the variable's component had at the time of a swizzle's load instead of the swizzle instead, which also preempts copy-prop from getting stuck in an infinite loop, as which the added tests. -- v2: vkd3d-shader/hlsl: Use values at the time of the swizzle's load in copy-propagation. vkd3d-shader/hlsl: Record trace of stored values in copy-propagation. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 74 ++++++++++++++++---------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 5a70878bc..3d39fb921 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1203,6 +1203,43 @@ static bool lower_broadcasts(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, s return false; } +/* Allocate a unique, ordered index to each instruction, which will be used for + * computing liveness ranges. + * Index 0 means unused; index 1 means function entry, so start at 2. */ +static unsigned int index_instructions(struct hlsl_block *block, unsigned int index) +{ + struct hlsl_ir_node *instr; + + LIST_FOR_EACH_ENTRY(instr, &block->instrs, struct hlsl_ir_node, entry) + { + instr->index = index++; + + if (instr->type == HLSL_IR_IF) + { + struct hlsl_ir_if *iff = hlsl_ir_if(instr); + index = index_instructions(&iff->then_block, index); + index = index_instructions(&iff->else_block, index); + } + else if (instr->type == HLSL_IR_LOOP) + { + index = index_instructions(&hlsl_ir_loop(instr)->body, index); + hlsl_ir_loop(instr)->next_index = index; + } + else if (instr->type == HLSL_IR_SWITCH) + { + struct hlsl_ir_switch *s = hlsl_ir_switch(instr); + struct hlsl_ir_switch_case *c; + + LIST_FOR_EACH_ENTRY(c, &s->cases, struct hlsl_ir_switch_case, entry) + { + index = index_instructions(&c->body, index); + } + } + } + + return index; +} + /* * Copy propagation. The basic idea is to recognize instruction sequences of the * form: @@ -3242,42 +3279,6 @@ static bool dce(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) return false; } -/* Allocate a unique, ordered index to each instruction, which will be used for - * computing liveness ranges. */ -static unsigned int index_instructions(struct hlsl_block *block, unsigned int index) -{ - struct hlsl_ir_node *instr; - - LIST_FOR_EACH_ENTRY(instr, &block->instrs, struct hlsl_ir_node, entry) - { - instr->index = index++; - - if (instr->type == HLSL_IR_IF) - { - struct hlsl_ir_if *iff = hlsl_ir_if(instr); - index = index_instructions(&iff->then_block, index); - index = index_instructions(&iff->else_block, index); - } - else if (instr->type == HLSL_IR_LOOP) - { - index = index_instructions(&hlsl_ir_loop(instr)->body, index); - hlsl_ir_loop(instr)->next_index = index; - } - else if (instr->type == HLSL_IR_SWITCH) - { - struct hlsl_ir_switch *s = hlsl_ir_switch(instr); - struct hlsl_ir_switch_case *c; - - LIST_FOR_EACH_ENTRY(c, &s->cases, struct hlsl_ir_switch_case, entry) - { - index = index_instructions(&c->body, index); - } - } - } - - return index; -} - static void dump_function(struct rb_entry *entry, void *context) { struct hlsl_ir_function *func = RB_ENTRY_VALUE(entry, struct hlsl_ir_function, entry); @@ -3520,7 +3521,6 @@ static void compute_liveness(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl struct hlsl_scope *scope; struct hlsl_ir_var *var; - /* Index 0 means unused; index 1 means function entry, so start at 2. */ index_instructions(&entry_func->body, 2); LIST_FOR_EACH_ENTRY(scope, &ctx->scopes, struct hlsl_scope, entry) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487
From: Francisco Casas <fcasas(a)codeweavers.com> Instead of only storing the value that each variable's component has at the moment of the instruction currently handled by copy-prop, we store the trace of all the historic values with their timestamps, i.e. the instruction index on which the value was stored. This would allow us to query the value that the variable had at the time of execution of previous instructions. --- libs/vkd3d-shader/hlsl_codegen.c | 170 ++++++++++++++++++++----------- 1 file changed, 112 insertions(+), 58 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 3d39fb921..013ec5703 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1204,7 +1204,7 @@ static bool lower_broadcasts(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, s } /* Allocate a unique, ordered index to each instruction, which will be used for - * computing liveness ranges. + * copy propagation and computing liveness ranges. * Index 0 means unused; index 1 means function entry, so start at 2. */ static unsigned int index_instructions(struct hlsl_block *block, unsigned int index) { @@ -1289,25 +1289,25 @@ static unsigned int index_instructions(struct hlsl_block *block, unsigned int in * cannot easily vectorize the stores @3 and @6. */ -enum copy_propagation_value_state -{ - VALUE_STATE_NOT_WRITTEN = 0, - VALUE_STATE_STATICALLY_WRITTEN, - VALUE_STATE_DYNAMICALLY_WRITTEN, -}; - struct copy_propagation_value { - enum copy_propagation_value_state state; + unsigned int timestamp; + bool statically_written; struct hlsl_ir_node *node; unsigned int component; }; +struct copy_propagation_component_trace +{ + struct copy_propagation_value *records; + size_t record_count, record_capacity; +}; + struct copy_propagation_var_def { struct rb_entry entry; struct hlsl_ir_var *var; - struct copy_propagation_value values[]; + struct copy_propagation_component_trace traces[]; }; struct copy_propagation_state @@ -1327,12 +1327,30 @@ static int copy_propagation_var_def_compare(const void *key, const struct rb_ent static void copy_propagation_var_def_destroy(struct rb_entry *entry, void *context) { struct copy_propagation_var_def *var_def = RB_ENTRY_VALUE(entry, struct copy_propagation_var_def, entry); + unsigned int component_count = hlsl_type_component_count(var_def->var->data_type); + unsigned int i; + for (i = 0; i < component_count; ++i) + vkd3d_free(var_def->traces[i].records); vkd3d_free(var_def); } +static struct copy_propagation_value *copy_propagation_get_value_at_time( + struct copy_propagation_component_trace *trace, unsigned int time) +{ + int r; + + for (r = trace->record_count - 1; r >= 0; --r) + { + if (trace->records[r].timestamp < time) + return &trace->records[r]; + } + + return NULL; +} + static struct copy_propagation_value *copy_propagation_get_value(const struct copy_propagation_state *state, - const struct hlsl_ir_var *var, unsigned int component) + const struct hlsl_ir_var *var, unsigned int component, unsigned int time) { for (; state; state = state->parent) { @@ -1341,20 +1359,18 @@ static struct copy_propagation_value *copy_propagation_get_value(const struct co { struct copy_propagation_var_def *var_def = RB_ENTRY_VALUE(entry, struct copy_propagation_var_def, entry); unsigned int component_count = hlsl_type_component_count(var->data_type); - enum copy_propagation_value_state state; + struct copy_propagation_value *value; assert(component < component_count); - state = var_def->values[component].state; + value = copy_propagation_get_value_at_time(&var_def->traces[component], time); - switch (state) - { - case VALUE_STATE_STATICALLY_WRITTEN: - return &var_def->values[component]; - case VALUE_STATE_DYNAMICALLY_WRITTEN: - return NULL; - case VALUE_STATE_NOT_WRITTEN: - break; - } + if (!value) + continue; + + if (value->statically_written) + return value; + else + return NULL; } } @@ -1372,7 +1388,7 @@ static struct copy_propagation_var_def *copy_propagation_create_var_def(struct h if (entry) return RB_ENTRY_VALUE(entry, struct copy_propagation_var_def, entry); - if (!(var_def = hlsl_alloc(ctx, offsetof(struct copy_propagation_var_def, values[component_count])))) + if (!(var_def = hlsl_alloc(ctx, offsetof(struct copy_propagation_var_def, traces[component_count])))) return NULL; var_def->var = var; @@ -1383,8 +1399,8 @@ static struct copy_propagation_var_def *copy_propagation_create_var_def(struct h return var_def; } -static void copy_propagation_invalidate_variable(struct copy_propagation_var_def *var_def, - unsigned int comp, unsigned char writemask) +static void copy_propagation_invalidate_variable(struct hlsl_ctx *ctx, struct copy_propagation_var_def *var_def, + unsigned int comp, unsigned char writemask, unsigned int time) { unsigned i; @@ -1393,13 +1409,34 @@ static void copy_propagation_invalidate_variable(struct copy_propagation_var_def for (i = 0; i < 4; ++i) { if (writemask & (1u << i)) - var_def->values[comp + i].state = VALUE_STATE_DYNAMICALLY_WRITTEN; + { + struct copy_propagation_component_trace *trace = &var_def->traces[comp + i]; + + /* Don't add an invalidate record if it is already invalidated. */ + if (trace->record_count && trace->records[trace->record_count - 1].timestamp == time) + { + assert(!trace->records[trace->record_count - 1].statically_written); + continue; + } + + assert(!trace->record_count || trace->records[trace->record_count - 1].timestamp < time); + + if (!hlsl_array_reserve(ctx, (void **)&trace->records, &trace->record_capacity, + trace->record_count + 1, sizeof(trace->records[0]))) + return; + + trace->records[trace->record_count].timestamp = time; + trace->records[trace->record_count].statically_written = false; + + ++trace->record_count; + } } } static void copy_propagation_invalidate_variable_from_deref_recurse(struct hlsl_ctx *ctx, struct copy_propagation_var_def *var_def, const struct hlsl_deref *deref, - struct hlsl_type *type, unsigned int depth, unsigned int comp_start, unsigned char writemask) + struct hlsl_type *type, unsigned int depth, unsigned int comp_start, unsigned char writemask, + unsigned int time) { unsigned int i, subtype_comp_count; struct hlsl_ir_node *path_node; @@ -1407,7 +1444,7 @@ static void copy_propagation_invalidate_variable_from_deref_recurse(struct hlsl_ if (depth == deref->path_len) { - copy_propagation_invalidate_variable(var_def, comp_start, writemask); + copy_propagation_invalidate_variable(ctx, var_def, comp_start, writemask, time); return; } @@ -1422,7 +1459,7 @@ static void copy_propagation_invalidate_variable_from_deref_recurse(struct hlsl_ comp_start += hlsl_type_component_count(type->e.record.fields[i].type); copy_propagation_invalidate_variable_from_deref_recurse(ctx, var_def, deref, subtype, - depth + 1, comp_start, writemask); + depth + 1, comp_start, writemask, time); } else { @@ -1431,28 +1468,30 @@ static void copy_propagation_invalidate_variable_from_deref_recurse(struct hlsl_ if (path_node->type == HLSL_IR_CONSTANT) { copy_propagation_invalidate_variable_from_deref_recurse(ctx, var_def, deref, subtype, - depth + 1, hlsl_ir_constant(path_node)->value.u[0].u * subtype_comp_count, writemask); + depth + 1, hlsl_ir_constant(path_node)->value.u[0].u * subtype_comp_count, + writemask, time); } else { for (i = 0; i < hlsl_type_element_count(type); ++i) { copy_propagation_invalidate_variable_from_deref_recurse(ctx, var_def, deref, subtype, - depth + 1, i * subtype_comp_count, writemask); + depth + 1, i * subtype_comp_count, writemask, time); } } } } static void copy_propagation_invalidate_variable_from_deref(struct hlsl_ctx *ctx, - struct copy_propagation_var_def *var_def, const struct hlsl_deref *deref, unsigned char writemask) + struct copy_propagation_var_def *var_def, const struct hlsl_deref *deref, + unsigned char writemask, unsigned int time) { copy_propagation_invalidate_variable_from_deref_recurse(ctx, var_def, deref, deref->var->data_type, - 0, 0, writemask); + 0, 0, writemask, time); } -static void copy_propagation_set_value(struct copy_propagation_var_def *var_def, unsigned int comp, - unsigned char writemask, struct hlsl_ir_node *instr) +static void copy_propagation_set_value(struct hlsl_ctx *ctx, struct copy_propagation_var_def *var_def, + unsigned int comp, unsigned char writemask, struct hlsl_ir_node *instr, unsigned int time) { unsigned int i, j = 0; @@ -1460,11 +1499,22 @@ static void copy_propagation_set_value(struct copy_propagation_var_def *var_def, { if (writemask & (1u << i)) { + struct copy_propagation_component_trace *trace = &var_def->traces[comp + i]; + + assert(!trace->record_count || trace->records[trace->record_count - 1].timestamp < time); + + if (!hlsl_array_reserve(ctx, (void **)&trace->records, &trace->record_capacity, + trace->record_count + 1, sizeof(trace->records[0]))) + return; + TRACE("Variable %s[%u] is written by instruction %p%s.\n", var_def->var->name, comp + i, instr, debug_hlsl_writemask(1u << i)); - var_def->values[comp + i].state = VALUE_STATE_STATICALLY_WRITTEN; - var_def->values[comp + i].node = instr; - var_def->values[comp + i].component = j++; + trace->records[trace->record_count].timestamp = time; + trace->records[trace->record_count].statically_written = true; + trace->records[trace->record_count].node = instr; + trace->records[trace->record_count].component = j++; + + ++trace->record_count; } } } @@ -1486,7 +1536,8 @@ static bool copy_propagation_replace_with_single_instr(struct hlsl_ctx *ctx, { struct copy_propagation_value *value; - if (!(value = copy_propagation_get_value(state, var, start + hlsl_swizzle_get_component(swizzle, i)))) + if (!(value = copy_propagation_get_value(state, var, start + hlsl_swizzle_get_component(swizzle, i), + instr->index))) return false; if (!new_instr) @@ -1537,8 +1588,8 @@ static bool copy_propagation_replace_with_constant_vector(struct hlsl_ctx *ctx, { struct copy_propagation_value *value; - if (!(value = copy_propagation_get_value(state, var, start + hlsl_swizzle_get_component(swizzle, i))) - || value->node->type != HLSL_IR_CONSTANT) + if (!(value = copy_propagation_get_value(state, var, start + hlsl_swizzle_get_component(swizzle, i), + instr->index)) || value->node->type != HLSL_IR_CONSTANT) return false; values.u[i] = hlsl_ir_constant(value->node)->value.u[value->component]; @@ -1603,7 +1654,7 @@ static bool copy_propagation_transform_swizzle(struct hlsl_ctx *ctx, } static bool copy_propagation_transform_object_load(struct hlsl_ctx *ctx, - struct hlsl_deref *deref, struct copy_propagation_state *state) + struct hlsl_deref *deref, struct copy_propagation_state *state, unsigned int time) { struct copy_propagation_value *value; struct hlsl_ir_load *load; @@ -1613,7 +1664,7 @@ static bool copy_propagation_transform_object_load(struct hlsl_ctx *ctx, return false; assert(count == 1); - if (!(value = copy_propagation_get_value(state, deref->var, start))) + if (!(value = copy_propagation_get_value(state, deref->var, start, time))) return false; assert(value->component == 0); @@ -1651,9 +1702,9 @@ static bool copy_propagation_transform_resource_load(struct hlsl_ctx *ctx, { bool progress = false; - progress |= copy_propagation_transform_object_load(ctx, &load->resource, state); + progress |= copy_propagation_transform_object_load(ctx, &load->resource, state, load->node.index); if (load->sampler.var) - progress |= copy_propagation_transform_object_load(ctx, &load->sampler, state); + progress |= copy_propagation_transform_object_load(ctx, &load->sampler, state, load->node.index); return progress; } @@ -1662,7 +1713,7 @@ static bool copy_propagation_transform_resource_store(struct hlsl_ctx *ctx, { bool progress = false; - progress |= copy_propagation_transform_object_load(ctx, &store->resource, state); + progress |= copy_propagation_transform_object_load(ctx, &store->resource, state, store->node.index); return progress; } @@ -1683,11 +1734,12 @@ static void copy_propagation_record_store(struct hlsl_ctx *ctx, struct hlsl_ir_s if (store->rhs.node->data_type->class == HLSL_CLASS_OBJECT) writemask = VKD3DSP_WRITEMASK_0; - copy_propagation_set_value(var_def, start, writemask, store->rhs.node); + copy_propagation_set_value(ctx, var_def, start, writemask, store->rhs.node, store->node.index); } else { - copy_propagation_invalidate_variable_from_deref(ctx, var_def, lhs, store->writemask); + copy_propagation_invalidate_variable_from_deref(ctx, var_def, lhs, store->writemask, + store->node.index); } } @@ -1704,7 +1756,7 @@ static void copy_propagation_state_destroy(struct copy_propagation_state *state) } static void copy_propagation_invalidate_from_block(struct hlsl_ctx *ctx, struct copy_propagation_state *state, - struct hlsl_block *block) + struct hlsl_block *block, unsigned int time) { struct hlsl_ir_node *instr; @@ -1722,7 +1774,7 @@ static void copy_propagation_invalidate_from_block(struct hlsl_ctx *ctx, struct if (!(var_def = copy_propagation_create_var_def(ctx, state, var))) continue; - copy_propagation_invalidate_variable_from_deref(ctx, var_def, lhs, store->writemask); + copy_propagation_invalidate_variable_from_deref(ctx, var_def, lhs, store->writemask, time); break; } @@ -1731,8 +1783,8 @@ static void copy_propagation_invalidate_from_block(struct hlsl_ctx *ctx, struct { struct hlsl_ir_if *iff = hlsl_ir_if(instr); - copy_propagation_invalidate_from_block(ctx, state, &iff->then_block); - copy_propagation_invalidate_from_block(ctx, state, &iff->else_block); + copy_propagation_invalidate_from_block(ctx, state, &iff->then_block, time); + copy_propagation_invalidate_from_block(ctx, state, &iff->else_block, time); break; } @@ -1741,7 +1793,7 @@ static void copy_propagation_invalidate_from_block(struct hlsl_ctx *ctx, struct { struct hlsl_ir_loop *loop = hlsl_ir_loop(instr); - copy_propagation_invalidate_from_block(ctx, state, &loop->body); + copy_propagation_invalidate_from_block(ctx, state, &loop->body, time); break; } @@ -1753,7 +1805,7 @@ static void copy_propagation_invalidate_from_block(struct hlsl_ctx *ctx, struct LIST_FOR_EACH_ENTRY(c, &s->cases, struct hlsl_ir_switch_case, entry) { - copy_propagation_invalidate_from_block(ctx, state, &c->body); + copy_propagation_invalidate_from_block(ctx, state, &c->body, time); } break; @@ -1786,8 +1838,8 @@ static bool copy_propagation_process_if(struct hlsl_ctx *ctx, struct hlsl_ir_if * touched in the two inner states, but this doesn't work for * loops (because we need to know what is invalidated in advance), * so we need copy_propagation_invalidate_from_block() anyway. */ - copy_propagation_invalidate_from_block(ctx, state, &iff->then_block); - copy_propagation_invalidate_from_block(ctx, state, &iff->else_block); + copy_propagation_invalidate_from_block(ctx, state, &iff->then_block, iff->node.index); + copy_propagation_invalidate_from_block(ctx, state, &iff->else_block, iff->node.index); return progress; } @@ -1798,7 +1850,7 @@ static bool copy_propagation_process_loop(struct hlsl_ctx *ctx, struct hlsl_ir_l struct copy_propagation_state inner_state; bool progress = false; - copy_propagation_invalidate_from_block(ctx, state, &loop->body); + copy_propagation_invalidate_from_block(ctx, state, &loop->body, loop->node.index); copy_propagation_state_init(ctx, &inner_state, state); progress |= copy_propagation_transform_block(ctx, &loop->body, &inner_state); @@ -1823,7 +1875,7 @@ static bool copy_propagation_process_switch(struct hlsl_ctx *ctx, struct hlsl_ir LIST_FOR_EACH_ENTRY(c, &s->cases, struct hlsl_ir_switch_case, entry) { - copy_propagation_invalidate_from_block(ctx, state, &c->body); + copy_propagation_invalidate_from_block(ctx, state, &c->body, s->node.index); } return progress; @@ -1884,6 +1936,8 @@ bool hlsl_copy_propagation_execute(struct hlsl_ctx *ctx, struct hlsl_block *bloc struct copy_propagation_state state; bool progress; + index_instructions(block, 2); + copy_propagation_state_init(ctx, &state, NULL); progress = copy_propagation_transform_block(ctx, block, &state); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487
From: Francisco Casas <fcasas(a)codeweavers.com> This also helps us to avoid replacing a swizzle with itself in copy-prop which can result in infinite loops, as with the included tests this commit. Consider the following sequence of instructions: 1 : A 2 : B = @1 3 : B 4 : A = @3 5 : @1.x Current copy-prop would replace 5 so it points to @3 now: 1 : A 2 : B = @1 3 : B 4 : A = @3 5 : @3.x But in the next iteration it would make it point back to @1, keeping it spinning infinitively. The solution is to index the instructions and don't replace the swizzle if the new load happens after the current load. --- Makefile.am | 1 + libs/vkd3d-shader/hlsl_codegen.c | 20 ++++++----- tests/hlsl/hard-copy-prop.shader_test | 49 +++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 8 deletions(-) create mode 100644 tests/hlsl/hard-copy-prop.shader_test diff --git a/Makefile.am b/Makefile.am index 7d5898193..a7bfeec8a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -103,6 +103,7 @@ vkd3d_shader_tests = \ tests/hlsl/gather.shader_test \ tests/hlsl/getdimensions.shader_test \ tests/hlsl/half.shader_test \ + tests/hlsl/hard-copy-prop.shader_test \ tests/hlsl/initializer-flatten.shader_test \ tests/hlsl/initializer-implicit-array.shader_test \ tests/hlsl/initializer-invalid-arg-count.shader_test \ diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 013ec5703..d5dadd5d7 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1521,7 +1521,7 @@ static void copy_propagation_set_value(struct hlsl_ctx *ctx, struct copy_propaga static bool copy_propagation_replace_with_single_instr(struct hlsl_ctx *ctx, const struct copy_propagation_state *state, const struct hlsl_deref *deref, - unsigned int swizzle, struct hlsl_ir_node *instr) + unsigned int swizzle, struct hlsl_ir_node *instr, unsigned int time) { const unsigned int instr_component_count = hlsl_type_component_count(instr->data_type); const struct hlsl_ir_var *var = deref->var; @@ -1537,7 +1537,7 @@ static bool copy_propagation_replace_with_single_instr(struct hlsl_ctx *ctx, struct copy_propagation_value *value; if (!(value = copy_propagation_get_value(state, var, start + hlsl_swizzle_get_component(swizzle, i), - instr->index))) + time))) return false; if (!new_instr) @@ -1573,7 +1573,7 @@ static bool copy_propagation_replace_with_single_instr(struct hlsl_ctx *ctx, static bool copy_propagation_replace_with_constant_vector(struct hlsl_ctx *ctx, const struct copy_propagation_state *state, const struct hlsl_deref *deref, - unsigned int swizzle, struct hlsl_ir_node *instr) + unsigned int swizzle, struct hlsl_ir_node *instr, unsigned int time) { const unsigned int instr_component_count = hlsl_type_component_count(instr->data_type); const struct hlsl_ir_var *var = deref->var; @@ -1589,7 +1589,7 @@ static bool copy_propagation_replace_with_constant_vector(struct hlsl_ctx *ctx, struct copy_propagation_value *value; if (!(value = copy_propagation_get_value(state, var, start + hlsl_swizzle_get_component(swizzle, i), - instr->index)) || value->node->type != HLSL_IR_CONSTANT) + time)) || value->node->type != HLSL_IR_CONSTANT) return false; values.u[i] = hlsl_ir_constant(value->node)->value.u[value->component]; @@ -1626,10 +1626,12 @@ static bool copy_propagation_transform_load(struct hlsl_ctx *ctx, return false; } - if (copy_propagation_replace_with_constant_vector(ctx, state, &load->src, HLSL_SWIZZLE(X, Y, Z, W), &load->node)) + if (copy_propagation_replace_with_constant_vector(ctx, state, &load->src, HLSL_SWIZZLE(X, Y, Z, W), + &load->node, load->node.index)) return true; - if (copy_propagation_replace_with_single_instr(ctx, state, &load->src, HLSL_SWIZZLE(X, Y, Z, W), &load->node)) + if (copy_propagation_replace_with_single_instr(ctx, state, &load->src, HLSL_SWIZZLE(X, Y, Z, W), + &load->node, load->node.index)) return true; return false; @@ -1644,10 +1646,12 @@ static bool copy_propagation_transform_swizzle(struct hlsl_ctx *ctx, return false; load = hlsl_ir_load(swizzle->val.node); - if (copy_propagation_replace_with_constant_vector(ctx, state, &load->src, swizzle->swizzle, &swizzle->node)) + if (copy_propagation_replace_with_constant_vector(ctx, state, &load->src, swizzle->swizzle, + &swizzle->node, load->node.index)) return true; - if (copy_propagation_replace_with_single_instr(ctx, state, &load->src, swizzle->swizzle, &swizzle->node)) + if (copy_propagation_replace_with_single_instr(ctx, state, &load->src, swizzle->swizzle, + &swizzle->node, load->node.index)) return true; return false; diff --git a/tests/hlsl/hard-copy-prop.shader_test b/tests/hlsl/hard-copy-prop.shader_test new file mode 100644 index 000000000..5b8604b82 --- /dev/null +++ b/tests/hlsl/hard-copy-prop.shader_test @@ -0,0 +1,49 @@ +[pixel shader] +float cond; + +float4 main() : sv_target +{ + float2 r = {1, 2}; + float2 tmp; + + // invalidate r + if (cond) + r = float2(10, 20); + + tmp = r; + r = tmp; + return r.y; +} + +[test] +uniform 0 float 0.0 +draw quad +probe all rgba (2.0, 2.0, 2.0, 2.0) +uniform 0 float 1.0 +draw quad +probe all rgba (20.0, 20.0, 20.0, 20.0) + + +[pixel shader] +float cond; + +float4 main() : sv_target +{ + float2 r = {3, 4}; + + // invalidate r + if (cond) + r = float2(30, 40); + + r = r; + r = float2(1, r.y); + + return float4(r, 0, 0); +} +[test] +uniform 0 float 0.0 +draw quad +probe all rgba (1.0, 4.0, 0.0, 0.0) +uniform 0 float 1.0 +draw quad +probe all rgba (1.0, 40.0, 0.0, 0.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487
On Wed Nov 22 17:35:35 2023 +0000, Zebediah Figura wrote:
This is a single component, right? For clarity can we rename it to something that has "component" in it? Okay, I renamed it to `struct copy_propagation_component_trace`.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487#note_53385
On Wed Nov 22 18:59:22 2023 +0000, Francisco Casas wrote:
changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/487/diffs?diff_id=85230&start_sha=ccc8dd58d1e3c4aa4f8b31a64fe259cc25130c52#3cf804f245af47d51595ff932bf817c50967eea2_1356_1365) The idea is to, assuming that the records are sorted from oldest to newest, find the oldest record that happened before `time`, i.e. the record that was in effect at a given time, or indicate when there isn't any (this is the `r == -1` case).
My understanding is that the for-if anti-pattern is when the iteration is not necessary, and I think it is in this case. Albeit, I can make it more readable at the expense of making it more verbose, I added copy_propagation_get_value_at_time(). Also... holy moly! That `break` in ``` if (!value) break; ``` should have been a `continue` instead. In the current code it is a `break` but because it is inside a `switch` statement. Surprising that no test broke with this mistake, I fixed it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487#note_53384
On Wed Nov 22 17:35:36 2023 +0000, Zebediah Figura wrote:
As long as we're already passing load->src we might as well just pass the whole load and reduce the need for another argument. Ok, done.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487#note_53388
On Wed Nov 22 17:35:36 2023 +0000, Zebediah Figura wrote:
Having this as an optimization is fine, if perhaps unnecessary, but why compare the timestamp here? Why would that cause problems? This is mostly about having strong assertions than optimizing, since it is easy to mess up when we change copy-prop.
I am relying, both in copy_propagation_set_value() and here in copy_propagation_invalidate_variable(), in the following assertion: ``` assert(!trace->record_count || trace->records[trace->record_count - 1].timestamp < time); ``` which basically means that no record should be added before the last record, if there is any. Currently, there is only one exception to this rule: copy_propagation_invalidate_from_block() can actually try to add two or more invalidations at the same time stamp that was passed as argument (if the variable is dynamically written more than once in the block), so this `if` is here to handle this very specific case, and the optimization is a nicety. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487#note_53386
My understanding is that the for-if anti-pattern is when the iteration is not necessary, and I think it is in this case. Albeit, I can make it more readable at the expense of making it more verbose, I added copy_propagation_get_value_at_time().
Okay, apparently this is the name Raymond Chen gave to something else. What I mean is cases like ``` for (...) { if (...) found = true; } if (!found) ... ``` (or variations on this, e.g. checking the iterator after the loop instead of using a separate flag variable). I have a strong personal dislike for this pattern. I think it's always more idiomatic and simpler to have a lookup helper, that returns NULL or false or whatever if the thing isn't found. I.e. not getting rid of the loop, but making it into a helper function. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487#note_53398
On Wed Nov 22 18:59:23 2023 +0000, Francisco Casas wrote:
This is mostly about having strong assertions than optimizing, since it is easy to mess up when we change copy-prop. I am relying, both in copy_propagation_set_value() and here in copy_propagation_invalidate_variable(), in the following assertion: ``` assert(!trace->record_count || trace->records[trace->record_count - 1].timestamp < time); ``` which basically means that no record should be added before the last record, if there is any. Currently, there is only one exception to this rule: copy_propagation_invalidate_from_block() can actually try to add two or more invalidations at the same time stamp that was passed as argument (if the variable is dynamically written more than once in the block), so this `if` is here to handle this very specific case, and the optimization is a nicety. The assertion is good, but you could just as easily change the < to a <=, right?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487#note_53399
On Wed Nov 22 19:14:31 2023 +0000, Zebediah Figura wrote:
The assertion is good, but you could just as easily change the < to a <=, right? Yes, but with the if, besides the optimization, we are checking that if there is a entry at the current time, it is not statically written.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487#note_53402
On Wed Nov 22 19:13:18 2023 +0000, Zebediah Figura wrote:
My understanding is that the for-if anti-pattern is when the iteration is not necessary, and I think it is in this case. Albeit, I can make it more readable at the expense of making it more verbose, I added copy_propagation_get_value_at_time(). Okay, apparently this is the name Raymond Chen gave to something else. What I mean is cases like
for (...) { if (...) found = true; } if (!found) ...(or variations on this, e.g. checking the iterator after the loop instead of using a separate flag variable). I have a strong personal dislike for this pattern. I think it's always more idiomatic and simpler to have a lookup helper, that returns NULL or false or whatever if the thing isn't found. I.e. not getting rid of the loop, but making it into a helper function. Okay, I will try to remember to use lookup helpers in these cases, I agree that is more idiomatic.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487#note_53403
Yes, but with the if, besides the optimization, we are checking that if there is a entry at the current time, it is not statically written.
Well it is not that I think that that has a huge chance of happening. I can totally change it if you insist, I just don't see the disadvantage of it being there. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/487#note_53404
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Zebediah Figura (＠zfigura)