[Mesa-dev] [PATCH 2/3] glsl: Use hash tables for brw_fs_vector_splitting().

[Kenneth Graunke]

On Saturday, September 05, 2015 08:39:22 PM Timothy Arceri wrote:
> On Sat, 2015-09-05 at 02:21 -0700, Kenneth Graunke wrote:
> > Cuts compile/link time of the fragment shader in #91857 by 25%
> > (21.64 -> 16.28).
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91857
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  .../drivers/dri/i965/brw_fs_vector_splitting.cpp   | 48 ++++++++++++-------
> > ---
> >  1 file changed, 26 insertions(+), 22 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_vector_splitting.cpp
> > b/src/mesa/drivers/dri/i965/brw_fs_vector_splitting.cpp
> > index 96d4f37..ef1ff03 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_vector_splitting.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_vector_splitting.cpp
> > @@ -43,6 +43,7 @@
> >  #include "glsl/ir_visitor.h"
> >  #include "glsl/ir_rvalue_visitor.h"
> >  #include "glsl/glsl_types.h"
> > +#include "util/hash_table.h"
> >  
> >  static bool debug = false;
> >  
> > @@ -72,11 +73,13 @@ public:
> >     ir_vector_reference_visitor(void)
> >     {
> >        this->mem_ctx = ralloc_context(NULL);
> > -      this->variable_list.make_empty();
> > +      this->ht = _mesa_hash_table_create(mem_ctx, _mesa_hash_pointer,
> > +                                         _mesa_key_pointer_equal);
> >     }
> >  
> >     ~ir_vector_reference_visitor(void)
> >     {
> > +      _mesa_hash_table_destroy(ht, NULL);
> 
> Not a big deal but you created the table with mem_ctx so you dont need to do
> this right?

Oh, good point.  Will drop it.

> >        ralloc_free(mem_ctx);
> >     }
> >  
> > @@ -89,7 +92,7 @@ public:
> >     variable_entry *get_variable_entry(ir_variable *var);
> >  
> >     /* List of variable_entry */
> > -   exec_list variable_list;
> > +   struct hash_table *ht;
> >  
> >     void *mem_ctx;
> >  };
> > @@ -119,13 +122,12 @@
> > ir_vector_reference_visitor::get_variable_entry(ir_variable *var)
> >        break;
> >     }
> >  
> > -   foreach_in_list(variable_entry, entry, &variable_list) {
> > -      if (entry->var == var)
> > -	 return entry;
> > -   }
> > +   struct hash_entry *hte = _mesa_hash_table_search(ht, var);
> > +   if (hte)
> > +      return (struct variable_entry *) hte->data;
> >  
> >     variable_entry *entry = new(mem_ctx) variable_entry(var);
> > -   this->variable_list.push_tail(entry);
> > +   _mesa_hash_table_insert(ht, var, entry);
> >     return entry;
> >  }
> >  
> > @@ -195,9 +197,9 @@
> > ir_vector_reference_visitor::visit_enter(ir_function_signature *ir)
> >  
> >  class ir_vector_splitting_visitor : public ir_rvalue_visitor {
> >  public:
> > -   ir_vector_splitting_visitor(exec_list *vars)
> > +   ir_vector_splitting_visitor(struct hash_table *vars)
> >     {
> > -      this->variable_list = vars;
> > +      this->ht = vars;
> >     }
> >  
> >     virtual ir_visitor_status visit_leave(ir_assignment *);
> > @@ -205,7 +207,7 @@ public:
> >     void handle_rvalue(ir_rvalue **rvalue);
> >     variable_entry *get_splitting_entry(ir_variable *var);
> >  
> > -   exec_list *variable_list;
> > +   struct hash_table *ht;
> >  };
> >  
> >  variable_entry *
> > @@ -216,13 +218,8 @@
> > ir_vector_splitting_visitor::get_splitting_entry(ir_variable *var)
> >     if (!var->type->is_vector())
> >        return NULL;
> >  
> > -   foreach_in_list(variable_entry, entry, variable_list) {
> > -      if (entry->var == var) {
> > -	 return entry;
> > -      }
> > -   }
> > -
> > -   return NULL;
> > +   struct hash_entry *hte = _mesa_hash_table_search(ht, var);
> > +   return hte ? (struct variable_entry *) hte->data : NULL;
> >  }
> >  
> >  void
> > @@ -329,12 +326,16 @@ ir_vector_splitting_visitor::visit_leave(ir_assignment
> > *ir)
> >  bool
> >  brw_do_vector_splitting(exec_list *instructions)
> >  {
> > +   struct hash_entry *hte;
> > +
> >     ir_vector_reference_visitor refs;
> >  
> >     visit_list_elements(&refs, instructions);
> >  
> >     /* Trim out variables we can't split. */
> > -   foreach_in_list_safe(variable_entry, entry, &refs.variable_list) {
> > +   bool ht_empty = true;
> > +   hash_table_foreach(refs.ht, hte) {
> > +      struct variable_entry *entry = (struct variable_entry *) hte->data;
> >        if (debug) {
> >  	 fprintf(stderr, "vector %s@%p: whole_access %d\n",
> >                   entry->var->name, (void *) entry->var,
> > @@ -342,11 +343,13 @@ brw_do_vector_splitting(exec_list *instructions)
> >        }
> >  
> >        if (entry->whole_vector_access) {
> > -	 entry->remove();
> > +         _mesa_hash_table_remove(refs.ht, hte);
> > +      } else {
> > +         ht_empty = false;
> >        }
> >     }
> >  
> > -   if (refs.variable_list.is_empty())
> > +   if (ht_empty)
> 
> hash_table has an entries field I think you could check if its empty like
> this:
> 
> if (refs.ht->entries == 0)
> 
> Other than this and the comment higher up.
> 
> Reviewed-by: Timothy Arceri <t_arceri at yahoo.com.au>

Hey, thanks!  I knew there must be an obvious way to do that, but it was
eluding me at the time.  Replaced :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150910/954ad4d3/attachment.sig>