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

[Eric Anholt]

Caio Marcelo de Oliveira Filho <caio.oliveira at intel.com> writes:

> When handling 'if' in constant 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. This is similar to the
> change done for copy propagation code.
>
>     x = 1;
>     if (...) {
>         z = x;  // This would turn into z = 1.
>         x = 22; // x gets killed.
>     } else {
>         w = x;  // This would NOT turn into w = 1.
>     }
>
> With the change, we let constant propagation happen independently in
> the two branches and only then apply the killed values for the
> subsequent code.
>
> The new code use a single hash table for keeping the kills of both
> branches (the branches only write to it), and it gets deleted after we
> use -- instead of waiting for mem_ctx to collect it.
>
> NIR deals well with constant propagation, so it already covered for
> the missing ones that this patch fixes.
> ---
>  .../glsl/opt_constant_propagation.cpp         | 43 +++++++++++--------
>  1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/src/compiler/glsl/opt_constant_propagation.cpp b/src/compiler/glsl/opt_constant_propagation.cpp
> index 05dc71efb72..25db3622aba 100644
> --- a/src/compiler/glsl/opt_constant_propagation.cpp
> +++ b/src/compiler/glsl/opt_constant_propagation.cpp
> @@ -122,7 +122,7 @@ public:
>     void constant_folding(ir_rvalue **rvalue);
>     void constant_propagation(ir_rvalue **rvalue);
>     void kill(ir_variable *ir, unsigned write_mask);
> -   void handle_if_block(exec_list *instructions);
> +   void handle_if_block(exec_list *instructions, hash_table *kills, bool *killed_all);
>     void handle_rvalue(ir_rvalue **rvalue);
>  
>     /** List of acp_entry: The available constants to propagate */
> @@ -356,15 +356,14 @@ ir_constant_propagation_visitor::visit_enter(ir_call *ir)
>  }
>  
>  void
> -ir_constant_propagation_visitor::handle_if_block(exec_list *instructions)
> +ir_constant_propagation_visitor::handle_if_block(exec_list *instructions, hash_table *kills, bool *killed_all)
>  {
>     exec_list *orig_acp = this->acp;
>     hash_table *orig_kills = this->kills;
>     bool orig_killed_all = this->killed_all;
>  
>     this->acp = new(mem_ctx) exec_list;
> -   this->kills = _mesa_hash_table_create(mem_ctx, _mesa_hash_pointer,
> -                                         _mesa_key_pointer_equal);
> +   this->kills = kills;
>     this->killed_all = false;
>  
>     /* Populate the initial acp with a constant of the original */
> @@ -374,20 +373,10 @@ ir_constant_propagation_visitor::handle_if_block(exec_list *instructions)
>  
>     visit_list_elements(this, instructions);
>  
> -   if (this->killed_all) {
> -      orig_acp->make_empty();
> -   }
> -
> -   hash_table *new_kills = this->kills;
> +   *killed_all = this->killed_all;
>     this->kills = orig_kills;
>     this->acp = orig_acp;
> -   this->killed_all = this->killed_all || orig_killed_all;
> -
> -   hash_entry *htk;
> -   hash_table_foreach(new_kills, htk) {
> -      kill_entry *k = (kill_entry *) htk->data;
> -      kill(k->var, k->write_mask);
> -   }
> +   this->killed_all = orig_killed_all;
>  }
>  
>  ir_visitor_status
> @@ -396,8 +385,26 @@ ir_constant_propagation_visitor::visit_enter(ir_if *ir)
>     ir->condition->accept(this);
>     handle_rvalue(&ir->condition);
>  
> -   handle_if_block(&ir->then_instructions);
> -   handle_if_block(&ir->else_instructions);
> +   hash_table *new_kills = _mesa_hash_table_create(mem_ctx, _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);

Passing in the new_kills the second time and using it as the starting
kills looked strange, but since nobody else will kill from that list
until our block below, it seems like this patch does what it meant to.

Reviewed-by: Eric Anholt <eric at anholt.net>

Possible follow-up change for someone looking at compiler perf:
kill_entry doesn't seem to need to be a list since
4654439fdd766f79a78fe0d812fd916f5815e7e6, and we could probably just
store the write_mask in the entry->data field and not have struct
kill_entry at all!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180705/16a42cf2/attachment.sig>