On 06/27/2018 06:18 PM, Caio Marcelo de Oliveira Filho wrote:
> When handling 'if' in copy propagation, if a certain variable was
> killed when processing the first branch of the 'if', then the second
> would get any propagation from previous nodes.
>
> x = y;
> if (...) {
> z = x; // This would turn into z = y.
> x = 22; // x gets killed.
> } else {
> w = x; // This would NOT turn into w = y.
> }
>
> With the change, we let copy propagation happen independently in the
> two branches and only then apply the killed values for the subsequent
> code.
>
> Results for Skylake:
>
> total instructions in shared programs: 15238463 -> 15238503 (<.01%)
> instructions in affected programs: 10317 -> 10357 (0.39%)
> helped: 0
> HURT: 20
>
> total cycles in shared programs: 571868000 -> 571868028 (<.01%)
> cycles in affected programs: 43507 -> 43535 (0.06%)
> helped: 14
> HURT: 6
>
> The hurt instruction count is caused because the extra propagation
> causes an input variable to be read from two branches of an
> if (load_input intrinsic in NIR). Depending on the complexity of each
> branch this might be a win or not in terms of cycles.
I just sent out a patch (nir/opt_peephole_select: Don't try to remove
flow control around indirect loads) that deals with a similar sort of
thing. Were the cases you observed also indirect loads? Maybe we
should add those to the class of things that don't get copy propagated.
> ---
> src/compiler/glsl/opt_copy_propagation.cpp | 44 ++++++++++++----------
> 1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/src/compiler/glsl/opt_copy_propagation.cpp
> b/src/compiler/glsl/opt_copy_propagation.cpp
> index 206dffe4f1c..4ba5eedb2b1 100644
> --- a/src/compiler/glsl/opt_copy_propagation.cpp
> +++ b/src/compiler/glsl/opt_copy_propagation.cpp
> @@ -71,7 +71,7 @@ public:
>
> void add_copy(ir_assignment *ir);
> void kill(ir_variable *ir);
> - void handle_if_block(exec_list *instructions);
> + void handle_if_block(exec_list *instructions, set *kills, bool
> *killed_all);
>
> /** Hash of lhs->rhs: The available copies to propagate */
> hash_table *acp;
> @@ -207,14 +207,13 @@ ir_copy_propagation_visitor::visit_enter(ir_call *ir)
> }
>
> void
> -ir_copy_propagation_visitor::handle_if_block(exec_list *instructions)
> +ir_copy_propagation_visitor::handle_if_block(exec_list *instructions, set
> *kills, bool *killed_all)
> {
> hash_table *orig_acp = this->acp;
> set *orig_kills = this->kills;
> bool orig_killed_all = this->killed_all;
>
> - kills = _mesa_set_create(NULL, _mesa_hash_pointer,
> - _mesa_key_pointer_equal);
> + this->kills = kills;
> this->killed_all = false;
>
> /* Populate the initial acp with a copy of the original */
> @@ -222,22 +221,12 @@ ir_copy_propagation_visitor::handle_if_block(exec_list
> *instructions)
>
> visit_list_elements(this, instructions);
>
> - if (this->killed_all) {
> - _mesa_hash_table_clear(orig_acp, NULL);
> - }
> + _mesa_hash_table_destroy(acp, NULL);
> + *killed_all = this->killed_all;
>
> - set *new_kills = this->kills;
> this->kills = orig_kills;
> - _mesa_hash_table_destroy(acp, NULL);
> this->acp = orig_acp;
> - this->killed_all = this->killed_all || orig_killed_all;
> -
> - struct set_entry *s_entry;
> - set_foreach(new_kills, s_entry) {
> - kill((ir_variable *) s_entry->key);
> - }
> -
> - _mesa_set_destroy(new_kills, NULL);
> + this->killed_all = orig_killed_all;
> }
>
> ir_visitor_status
> @@ -245,8 +234,25 @@ ir_copy_propagation_visitor::visit_enter(ir_if *ir)
> {
> ir->condition->accept(this);
>
> - handle_if_block(&ir->then_instructions);
> - handle_if_block(&ir->else_instructions);
> + set *new_kills = _mesa_set_create(NULL, _mesa_hash_pointer,
> + _mesa_key_pointer_equal);
> + bool then_killed_all = false;
> + bool else_killed_all = false;
> +
> + handle_if_block(&ir->then_instructions, new_kills, &then_killed_all);
> + handle_if_block(&ir->else_instructions, new_kills, &else_killed_all);
> +
> + if (then_killed_all || else_killed_all) {
> + _mesa_hash_table_clear(acp, NULL);
> + killed_all = true;
> + } else {
> + struct set_entry *s_entry;
> + set_foreach(new_kills, s_entry) {
> + kill((ir_variable *) s_entry->key);
> + }
> + }
> +
> + _mesa_set_destroy(new_kills, NULL);
>
> /* handle_if_block() already descended into the children. */
> return visit_continue_with_parent;
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
