[Mesa-dev] [PATCH 4/4] glsl: Use hash table cloning in copy propagation

[Tapani Pälli]

On 01/12/2017 09:23 PM, Thomas Helland wrote:
> Walking the whole hash table, inserting entries by hashing them first
> is just a really really bad idea. We can simply memcpy the whole thing.

Maybe it is just 'really' not 'really really' since I don't spot any 
difference in time running the torture test in bug #94477 (oscillates 
close to 120s with both with and without these patches), I would expect 
at least some difference as it is utilizing this path a lot. Did you 
measure performance difference?


> This eliminates hash_table_insert from perf's top 15 costly functions.
> ---
>  src/compiler/glsl/opt_copy_propagation.cpp         | 17 ++++---------
>  .../glsl/opt_copy_propagation_elements.cpp         | 29 ++++++++--------------
>  2 files changed, 15 insertions(+), 31 deletions(-)
>
> diff --git a/src/compiler/glsl/opt_copy_propagation.cpp b/src/compiler/glsl/opt_copy_propagation.cpp
> index 247c4988ed..4fd4aa8d82 100644
> --- a/src/compiler/glsl/opt_copy_propagation.cpp
> +++ b/src/compiler/glsl/opt_copy_propagation.cpp
> @@ -202,16 +202,11 @@ ir_copy_propagation_visitor::handle_if_block(exec_list *instructions)
>     exec_list *orig_kills = this->kills;
>     bool orig_killed_all = this->killed_all;
>
> -   acp = _mesa_hash_table_create(NULL, _mesa_hash_pointer,
> -                                 _mesa_key_pointer_equal);
>     this->kills = new(mem_ctx) exec_list;
>     this->killed_all = false;
>
>     /* Populate the initial acp with a copy of the original */
> -   struct hash_entry *entry;
> -   hash_table_foreach(orig_acp, entry) {
> -      _mesa_hash_table_insert(acp, entry->key, entry->data);
> -   }
> +   acp = _mesa_hash_table_clone(orig_acp, NULL);
>
>     visit_list_elements(this, instructions);
>
> @@ -251,16 +246,14 @@ ir_copy_propagation_visitor::handle_loop(ir_loop *ir, bool keep_acp)
>     exec_list *orig_kills = this->kills;
>     bool orig_killed_all = this->killed_all;
>
> -   acp = _mesa_hash_table_create(NULL, _mesa_hash_pointer,
> -                                 _mesa_key_pointer_equal);
>     this->kills = new(mem_ctx) exec_list;
>     this->killed_all = false;
>
>     if (keep_acp) {
> -      struct hash_entry *entry;
> -      hash_table_foreach(orig_acp, entry) {
> -         _mesa_hash_table_insert(acp, entry->key, entry->data);
> -      }
> +      acp = _mesa_hash_table_clone(orig_acp, NULL);
> +   } else {
> +      acp = _mesa_hash_table_create(NULL, _mesa_hash_pointer,
> +                                    _mesa_key_pointer_equal);
>     }
>
>     visit_list_elements(this, &ir->body_instructions);
> diff --git a/src/compiler/glsl/opt_copy_propagation_elements.cpp b/src/compiler/glsl/opt_copy_propagation_elements.cpp
> index 9f79fa9202..8bae424a1d 100644
> --- a/src/compiler/glsl/opt_copy_propagation_elements.cpp
> +++ b/src/compiler/glsl/opt_copy_propagation_elements.cpp
> @@ -124,6 +124,12 @@ public:
>        ralloc_free(mem_ctx);
>     }
>
> +   void clone_acp(hash_table *lhs, hash_table *rhs)
> +   {
> +      lhs_ht = _mesa_hash_table_clone(lhs, mem_ctx);
> +      rhs_ht = _mesa_hash_table_clone(rhs, mem_ctx);
> +   }
> +
>     void create_acp()
>     {
>        lhs_ht = _mesa_hash_table_create(mem_ctx, _mesa_hash_pointer,
> @@ -138,19 +144,6 @@ public:
>        _mesa_hash_table_destroy(rhs_ht, NULL);
>     }
>
> -   void populate_acp(hash_table *lhs, hash_table *rhs)
> -   {
> -      struct hash_entry *entry;
> -
> -      hash_table_foreach(lhs, entry) {
> -         _mesa_hash_table_insert(lhs_ht, entry->key, entry->data);
> -      }
> -
> -      hash_table_foreach(rhs, entry) {
> -         _mesa_hash_table_insert(rhs_ht, entry->key, entry->data);
> -      }
> -   }
> -
>     void handle_loop(ir_loop *, bool keep_acp);
>     virtual ir_visitor_status visit_enter(class ir_loop *);
>     virtual ir_visitor_status visit_enter(class ir_function_signature *);
> @@ -395,10 +388,8 @@ ir_copy_propagation_elements_visitor::handle_if_block(exec_list *instructions)
>     this->kills = new(mem_ctx) exec_list;
>     this->killed_all = false;
>
> -   create_acp();
> -
>     /* Populate the initial acp with a copy of the original */
> -   populate_acp(orig_lhs_ht, orig_rhs_ht);
> +   clone_acp(orig_lhs_ht, orig_rhs_ht);
>
>     visit_list_elements(this, instructions);
>
> @@ -454,11 +445,11 @@ ir_copy_propagation_elements_visitor::handle_loop(ir_loop *ir, bool keep_acp)
>     this->kills = new(mem_ctx) exec_list;
>     this->killed_all = false;
>
> -   create_acp();
> -
>     if (keep_acp) {
>        /* Populate the initial acp with a copy of the original */
> -      populate_acp(orig_lhs_ht, orig_rhs_ht);
> +      clone_acp(orig_lhs_ht, orig_rhs_ht);
> +   } else {
> +      create_acp();
>     }
>
>     visit_list_elements(this, &ir->body_instructions);
>