[Mesa-dev] [PATCH 03/10] glsl: don't let an 'if' then-branch kill copy propagation for else-branch

[Ian Romanick]

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;
>