[Mesa-dev] [PATCH] glsl: remove dead code in a single pass

[Thomas Helland]

2015-07-12 1:17 GMT+02:00 Timothy Arceri <t_arceri at yahoo.com.au>:
> Currently only one ir assignment is removed for each var in a single
> dead code optimisation pass. This means if a var has more than one
> assignment, then it requires all the glsl optimisations to be run again
> for each additional assignment to be removed.
> Another pass is also required to remove the variable itself.
>
> With this change all assignments and the variable are removed in a single
> pass.
>
> Some of the arrays of arrays conformance tests that were looping
> through 8 dimensions ended up with a var with hundreds of assignments.
>
> This change helps ES31-CTS.arrays_of_arrays.InteractionFunctionCalls1
> go from around 3 min 20 sec -> 2 min
>
> ES31-CTS.arrays_of_arrays.InteractionFunctionCalls2 went from
> around 9 min 20 sec to 7 min 30 sec
>
> I had difficulty getting the public shader-db to give a constent result
> with or without this change but the results seemed unchanged at between
> 15-20 seconds.
> ---
>  src/glsl/ir_variable_refcount.cpp | 23 ++++++++++++++++++++---
>  src/glsl/ir_variable_refcount.h   |  7 ++++++-
>  src/glsl/opt_dead_code.cpp        | 30 +++++++++++++++++++++---------
>  src/glsl/opt_tree_grafting.cpp    |  2 --
>  4 files changed, 47 insertions(+), 15 deletions(-)
>
> diff --git a/src/glsl/ir_variable_refcount.cpp b/src/glsl/ir_variable_refcount.cpp
> index e4d825c..cc13ac0 100644
> --- a/src/glsl/ir_variable_refcount.cpp
> +++ b/src/glsl/ir_variable_refcount.cpp
> @@ -46,6 +46,16 @@ static void
>  free_entry(struct hash_entry *entry)
>  {
>     ir_variable_refcount_entry *ivre = (ir_variable_refcount_entry *) entry->data;
> +
> +   /* Free assignment list */
> +   while (!ivre->assign_list.is_empty()) {
> +      struct assignment_entry *assignment_entry =
> +         exec_node_data(struct assignment_entry,
> +                        ivre->assign_list.head, link);
> +      assignment_entry->link.remove();
> +      free(assignment_entry);
> +   }
> +
>     delete ivre;
>  }
>
> @@ -59,7 +69,6 @@ ir_variable_refcount_visitor::~ir_variable_refcount_visitor()
>  ir_variable_refcount_entry::ir_variable_refcount_entry(ir_variable *var)
>  {
>     this->var = var;
> -   assign = NULL;
>     assigned_count = 0;
>     declaration = false;
>     referenced_count = 0;
> @@ -125,8 +134,16 @@ ir_variable_refcount_visitor::visit_leave(ir_assignment *ir)
>     entry = this->get_variable_entry(ir->lhs->variable_referenced());
>     if (entry) {
>        entry->assigned_count++;
> -      if (entry->assign == NULL)
> -        entry->assign = ir;
> +      /* Build a list for dead code optimisation. Don't bother adding any more
> +       * if there are more references then assignments as this means the
> +       * variable is used and won't be optimised out.
> +       */
> +      if (entry->referenced_count == entry->assigned_count) {
> +         struct assignment_entry *assignment_entry =
> +            (struct assignment_entry *)calloc(1, sizeof(*assignment_entry));
> +         assignment_entry->assign = ir;
> +         entry->assign_list.push_head(&assignment_entry->link);
> +      }
>     }
>
>     return visit_continue;
> diff --git a/src/glsl/ir_variable_refcount.h b/src/glsl/ir_variable_refcount.h
> index c15e8110..1fecd52 100644
> --- a/src/glsl/ir_variable_refcount.h
> +++ b/src/glsl/ir_variable_refcount.h
> @@ -33,13 +33,18 @@
>  #include "ir_visitor.h"
>  #include "glsl_types.h"
>
> +struct assignment_entry {
> +   exec_node link;
> +   ir_assignment *assign;
> +};
> +
>  class ir_variable_refcount_entry
>  {
>  public:
>     ir_variable_refcount_entry(ir_variable *var);
>
>     ir_variable *var; /* The key: the variable's pointer. */
> -   ir_assignment *assign; /* An assignment to the variable, if any */
> +   exec_list assign_list; /* List of assignments to the variable, if any */
>
>     /** Number of times the variable is referenced, including assignments. */
>     unsigned referenced_count;
> diff --git a/src/glsl/opt_dead_code.cpp b/src/glsl/opt_dead_code.cpp
> index f45bf5d..6d1f675 100644
> --- a/src/glsl/opt_dead_code.cpp
> +++ b/src/glsl/opt_dead_code.cpp
> @@ -75,22 +75,34 @@ do_dead_code(exec_list *instructions, bool uniform_locations_assigned)
>           || !entry->declaration)
>          continue;
>
> -      if (entry->assign) {
> -        /* Remove a single dead assignment to the variable we found.
> +      if (!entry->assign_list.is_empty()) {
> +        /* Remove all the dead assignments to the variable we found.
>           * Don't do so if it's a shader or function output, though.
>           */
>          if (entry->var->data.mode != ir_var_function_out &&
>              entry->var->data.mode != ir_var_function_inout &&
>               entry->var->data.mode != ir_var_shader_out) {
> -           entry->assign->remove();
> -           progress = true;
>
> -           if (debug) {
> -              printf("Removed assignment to %s@%p\n",
> -                     entry->var->name, (void *) entry->var);
> -           }
> +            while (!entry->assign_list.is_empty()) {
> +               struct assignment_entry *assignment_entry =
> +                  exec_node_data(struct assignment_entry,
> +                                 entry->assign_list.head, link);
> +
> +              assignment_entry->assign->remove();
> +
> +              if (debug) {
> +                 printf("Removed assignment to %s@%p\n",
> +                        entry->var->name, (void *) entry->var);
> +               }
> +
> +               assignment_entry->link.remove();
> +               free(assignment_entry);
> +            }
> +            progress = true;
>          }
> -      } else {
> +      }
> +
> +      if (entry->assign_list.is_empty()) {
>          /* If there are no assignments or references to the variable left,
>           * then we can remove its declaration.
>           */
> diff --git a/src/glsl/opt_tree_grafting.cpp b/src/glsl/opt_tree_grafting.cpp
> index d47613c..cda6447 100644
> --- a/src/glsl/opt_tree_grafting.cpp
> +++ b/src/glsl/opt_tree_grafting.cpp
> @@ -371,8 +371,6 @@ tree_grafting_basic_block(ir_instruction *bb_first,
>           entry->referenced_count != 2)
>          continue;
>
> -      assert(assign == entry->assign);
> -
>        /* Found a possibly graftable assignment.  Now, walk through the
>         * rest of the BB seeing if the deref is here, and if nothing interfered with
>         * pasting its expression's values in between.
> --
> 2.4.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Hi Timothy,

Just gave your patch a whirl through my shader-db.
Three runs on master and three runs with your path seems
to cut my shader-db run from aprox 117 secs to 112.
(I didn't do any statistics, just a quick look at the results.)
Also there is no changes in instruction counts,
which is to be expected, but nice to confirm.
Let me know if you want more thorough statistics.
If you want you can strap my "Tested-By" on this.

Thomas