[Mesa-dev] [PATCH] glsl: Remove CSE pass.

Ilia Mirkin imirkin at alum.mit.edu
Sat Oct 3 11:31:24 PDT 2015


On Sat, Oct 3, 2015 at 1:57 PM, Matt Turner <mattst88 at gmail.com> wrote:
> With NIR, it actually hurts things.

Buuutt.... not everything uses NIR. Of course I'm sure it has a
similarly negative effect with nouveau/codegen, but I'm not so sure
about things like nv30 which don't have an optimizing compiler
backend. IMHO there should be a bit somewhere in the compiler options
that indicates whether the driver implements an optimizing compiler,
and if so, then all these idiotic optimizations should be removed. [I
realize some are needed for correctness, obviously keep those.]

  -ilia

>
> total instructions in shared programs: 6529329 -> 6528888 (-0.01%)
> instructions in affected programs:     14833 -> 14392 (-2.97%)
> helped:                                299
> HURT:                                  1
>
> In all affected programs I inspected (including the single hurt one) the
> pass CSE'd some multiplies and caused some reassociation (e.g., caused
> (A * B) * C to be A * (B * C)) when the original intermediate result was
> reused elsewhere.
> ---
>  src/glsl/Makefile.sources       |   1 -
>  src/glsl/glsl_parser_extras.cpp |   1 -
>  src/glsl/ir_optimization.h      |   1 -
>  src/glsl/opt_cse.cpp            | 472 ----------------------------------------
>  4 files changed, 475 deletions(-)
>  delete mode 100644 src/glsl/opt_cse.cpp
>
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index 32b6dba..7083246 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -184,7 +184,6 @@ LIBGLSL_FILES = \
>         opt_constant_variable.cpp \
>         opt_copy_propagation.cpp \
>         opt_copy_propagation_elements.cpp \
> -       opt_cse.cpp \
>         opt_dead_builtin_variables.cpp \
>         opt_dead_builtin_varyings.cpp \
>         opt_dead_code.cpp \
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index f554241..b521c5f 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -1899,7 +1899,6 @@ do_common_optimization(exec_list *ir, bool linked,
>        progress = do_constant_variable_unlinked(ir) || progress;
>     progress = do_constant_folding(ir) || progress;
>     progress = do_minmax_prune(ir) || progress;
> -   progress = do_cse(ir) || progress;
>     progress = do_rebalance_tree(ir) || progress;
>     progress = do_algebraic(ir, native_integers, options) || progress;
>     progress = do_lower_jumps(ir) || progress;
> diff --git a/src/glsl/ir_optimization.h b/src/glsl/ir_optimization.h
> index 265b223..ce5c492 100644
> --- a/src/glsl/ir_optimization.h
> +++ b/src/glsl/ir_optimization.h
> @@ -87,7 +87,6 @@ bool do_constant_variable_unlinked(exec_list *instructions);
>  bool do_copy_propagation(exec_list *instructions);
>  bool do_copy_propagation_elements(exec_list *instructions);
>  bool do_constant_propagation(exec_list *instructions);
> -bool do_cse(exec_list *instructions);
>  void do_dead_builtin_varyings(struct gl_context *ctx,
>                                gl_shader *producer, gl_shader *consumer,
>                                unsigned num_tfeedback_decls,
> diff --git a/src/glsl/opt_cse.cpp b/src/glsl/opt_cse.cpp
> deleted file mode 100644
> index 4b8e9a0..0000000
> --- a/src/glsl/opt_cse.cpp
> +++ /dev/null
> @@ -1,472 +0,0 @@
> -/*
> - * Copyright © 2013 Intel Corporation
> - *
> - * Permission is hereby granted, free of charge, to any person obtaining a
> - * copy of this software and associated documentation files (the "Software"),
> - * to deal in the Software without restriction, including without limitation
> - * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> - * and/or sell copies of the Software, and to permit persons to whom the
> - * Software is furnished to do so, subject to the following conditions:
> - *
> - * The above copyright notice and this permission notice (including the next
> - * paragraph) shall be included in all copies or substantial portions of the
> - * Software.
> - *
> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> - * DEALINGS IN THE SOFTWARE.
> - */
> -
> -/**
> - * \file opt_cse.cpp
> - *
> - * constant subexpression elimination at the GLSL IR level.
> - *
> - * Compare to brw_fs_cse.cpp for a more complete CSE implementation.  This one
> - * is generic and handles texture operations, but it's rather simple currently
> - * and doesn't support modification of variables in the available expressions
> - * list, so it can't do variables other than uniforms or shader inputs.
> - */
> -
> -#include "ir.h"
> -#include "ir_visitor.h"
> -#include "ir_rvalue_visitor.h"
> -#include "ir_basic_block.h"
> -#include "ir_optimization.h"
> -#include "ir_builder.h"
> -#include "glsl_types.h"
> -
> -using namespace ir_builder;
> -
> -static bool debug = false;
> -
> -namespace {
> -
> -/**
> - * This is the record of an available expression for common subexpression
> - * elimination.
> - */
> -class ae_entry : public exec_node
> -{
> -public:
> -   ae_entry(ir_instruction *base_ir, ir_rvalue **val)
> -      : val(val), base_ir(base_ir)
> -   {
> -      assert(val);
> -      assert(*val);
> -      assert(base_ir);
> -
> -      var = NULL;
> -   }
> -
> -   void init(ir_instruction *base_ir, ir_rvalue **val)
> -   {
> -      this->val = val;
> -      this->base_ir = base_ir;
> -      this->var = NULL;
> -
> -      assert(val);
> -      assert(*val);
> -      assert(base_ir);
> -   }
> -
> -   /**
> -    * The pointer to the expression that we might be able to reuse
> -    *
> -    * Note the double pointer -- this is the place in the base_ir expression
> -    * tree that we would rewrite to move the expression out to a new variable
> -    * assignment.
> -    */
> -   ir_rvalue **val;
> -
> -   /**
> -    * Root instruction in the basic block where the expression appeared.
> -    *
> -    * This is used so that we can insert the new variable declaration into the
> -    * instruction stream (since *val is just somewhere in base_ir's expression
> -    * tree).
> -    */
> -   ir_instruction *base_ir;
> -
> -   /**
> -    * The variable that the expression has been stored in, if it's been CSEd
> -    * once already.
> -    */
> -   ir_variable *var;
> -};
> -
> -class cse_visitor : public ir_rvalue_visitor {
> -public:
> -   cse_visitor(exec_list *validate_instructions)
> -      : validate_instructions(validate_instructions)
> -   {
> -      progress = false;
> -      mem_ctx = ralloc_context(NULL);
> -      this->ae = new(mem_ctx) exec_list;
> -   }
> -   ~cse_visitor()
> -   {
> -      ralloc_free(mem_ctx);
> -   }
> -
> -   virtual ir_visitor_status visit_enter(ir_function_signature *ir);
> -   virtual ir_visitor_status visit_enter(ir_loop *ir);
> -   virtual ir_visitor_status visit_enter(ir_if *ir);
> -   virtual ir_visitor_status visit_enter(ir_call *ir);
> -   virtual void handle_rvalue(ir_rvalue **rvalue);
> -
> -   bool progress;
> -
> -private:
> -   void *mem_ctx;
> -
> -   ir_rvalue *try_cse(ir_rvalue *rvalue);
> -   void add_to_ae(ir_rvalue **rvalue);
> -
> -   /**
> -    * Move all nodes from the ae list to the free list
> -    */
> -   void empty_ae_list();
> -
> -   /**
> -    * Get and initialize a new ae_entry
> -    *
> -    * This will either come from the free list or be freshly allocated.
> -    */
> -   ae_entry *get_ae_entry(ir_rvalue **rvalue);
> -
> -   /** List of ae_entry: The available expressions to reuse */
> -   exec_list *ae;
> -
> -   /**
> -    * The whole shader, so that we can validate_ir_tree in debug mode.
> -    *
> -    * This proved quite useful when trying to get the tree manipulation
> -    * right.
> -    */
> -   exec_list *validate_instructions;
> -
> -   /**
> -    * List of available-for-use ae_entry objects.
> -    */
> -   exec_list free_ae_entries;
> -};
> -
> -/**
> - * Visitor to walk an expression tree to check that all variables referenced
> - * are constants.
> - */
> -class is_cse_candidate_visitor : public ir_hierarchical_visitor
> -{
> -public:
> -
> -   is_cse_candidate_visitor()
> -      : ok(true)
> -   {
> -   }
> -
> -   virtual ir_visitor_status visit(ir_dereference_variable *ir);
> -
> -   bool ok;
> -};
> -
> -
> -class contains_rvalue_visitor : public ir_rvalue_visitor
> -{
> -public:
> -
> -   contains_rvalue_visitor(ir_rvalue *val)
> -      : val(val)
> -   {
> -      found = false;
> -   }
> -
> -   virtual void handle_rvalue(ir_rvalue **rvalue);
> -
> -   bool found;
> -
> -private:
> -   ir_rvalue *val;
> -};
> -
> -} /* unnamed namespace */
> -
> -static void
> -dump_ae(exec_list *ae)
> -{
> -   int i = 0;
> -
> -   printf("CSE: AE contents:\n");
> -   foreach_in_list(ae_entry, entry, ae) {
> -      printf("CSE:   AE %2d (%p): ", i, entry);
> -      (*entry->val)->print();
> -      printf("\n");
> -
> -      if (entry->var)
> -         printf("CSE:     in var %p:\n", entry->var);
> -
> -      i++;
> -   }
> -}
> -
> -ir_visitor_status
> -is_cse_candidate_visitor::visit(ir_dereference_variable *ir)
> -{
> -   /* Currently, since we don't handle kills of the ae based on variables
> -    * getting assigned, we can only handle constant variables.
> -    */
> -   if (ir->var->data.read_only) {
> -      return visit_continue;
> -   } else {
> -      if (debug)
> -         printf("CSE: non-candidate: var %s is not read only\n", ir->var->name);
> -      ok = false;
> -      return visit_stop;
> -   }
> -}
> -
> -void
> -contains_rvalue_visitor::handle_rvalue(ir_rvalue **rvalue)
> -{
> -   if (*rvalue == val)
> -      found = true;
> -}
> -
> -static bool
> -contains_rvalue(ir_rvalue *haystack, ir_rvalue *needle)
> -{
> -   contains_rvalue_visitor v(needle);
> -   haystack->accept(&v);
> -   return v.found;
> -}
> -
> -static bool
> -is_cse_candidate(ir_rvalue *ir)
> -{
> -   /* Our temporary variable assignment generation isn't ready to handle
> -    * anything bigger than a vector.
> -    */
> -   if (!ir->type->is_vector() && !ir->type->is_scalar()) {
> -      if (debug)
> -         printf("CSE: non-candidate: not a vector/scalar\n");
> -      return false;
> -   }
> -
> -   /* Only handle expressions and textures currently.  We may want to extend
> -    * to variable-index array dereferences at some point.
> -    */
> -   switch (ir->ir_type) {
> -   case ir_type_expression:
> -   case ir_type_texture:
> -      break;
> -   default:
> -      if (debug)
> -         printf("CSE: non-candidate: not an expression/texture\n");
> -      return false;
> -   }
> -
> -   is_cse_candidate_visitor v;
> -
> -   ir->accept(&v);
> -
> -   return v.ok;
> -}
> -
> -/**
> - * Tries to find and return a reference to a previous computation of a given
> - * expression.
> - *
> - * Walk the list of available expressions checking if any of them match the
> - * rvalue, and if so, move the previous copy of the expression to a temporary
> - * and return a reference of the temporary.
> - */
> -ir_rvalue *
> -cse_visitor::try_cse(ir_rvalue *rvalue)
> -{
> -   foreach_in_list(ae_entry, entry, ae) {
> -      if (debug) {
> -         printf("Comparing to AE %p: ", entry);
> -         (*entry->val)->print();
> -         printf("\n");
> -      }
> -
> -      if (!rvalue->equals(*entry->val))
> -         continue;
> -
> -      if (debug) {
> -         printf("CSE: Replacing: ");
> -         (*entry->val)->print();
> -         printf("\n");
> -         printf("CSE:      with: ");
> -         rvalue->print();
> -         printf("\n");
> -      }
> -
> -      if (!entry->var) {
> -         ir_instruction *base_ir = entry->base_ir;
> -
> -         ir_variable *var = new(rvalue) ir_variable(rvalue->type,
> -                                                    "cse",
> -                                                    ir_var_temporary);
> -
> -         /* Write the previous expression result into a new variable. */
> -         base_ir->insert_before(var);
> -         ir_assignment *assignment = assign(var, *entry->val);
> -         base_ir->insert_before(assignment);
> -
> -         /* Replace the expression in the original tree with a deref of the
> -          * variable, but keep tracking the expression for further reuse.
> -          */
> -         *entry->val = new(rvalue) ir_dereference_variable(var);
> -         entry->val = &assignment->rhs;
> -
> -         entry->var = var;
> -
> -         /* Update the base_irs in the AE list.  We have to be sure that
> -          * they're correct -- expressions from our base_ir that weren't moved
> -          * need to stay in this base_ir (so that later consumption of them
> -          * puts new variables between our new variable and our base_ir), but
> -          * expressions from our base_ir that we *did* move need base_ir
> -          * updated so that any further elimination from inside gets its new
> -          * assignments put before our new assignment.
> -          */
> -         foreach_in_list(ae_entry, fixup_entry, ae) {
> -            if (contains_rvalue(assignment->rhs, *fixup_entry->val))
> -               fixup_entry->base_ir = assignment;
> -         }
> -
> -         if (debug)
> -            dump_ae(ae);
> -      }
> -
> -      /* Replace the expression in our current tree with the variable. */
> -      return new(rvalue) ir_dereference_variable(entry->var);
> -   }
> -
> -   return NULL;
> -}
> -
> -void
> -cse_visitor::empty_ae_list()
> -{
> -   free_ae_entries.append_list(ae);
> -}
> -
> -ae_entry *
> -cse_visitor::get_ae_entry(ir_rvalue **rvalue)
> -{
> -   ae_entry *entry = (ae_entry *) free_ae_entries.pop_head();
> -   if (entry) {
> -      entry->init(base_ir, rvalue);
> -   } else {
> -      entry = new(mem_ctx) ae_entry(base_ir, rvalue);
> -   }
> -
> -   return entry;
> -}
> -
> -/** Add the rvalue to the list of available expressions for CSE. */
> -void
> -cse_visitor::add_to_ae(ir_rvalue **rvalue)
> -{
> -   if (debug) {
> -      printf("CSE: Add to AE: ");
> -      (*rvalue)->print();
> -      printf("\n");
> -   }
> -
> -   ae->push_tail(get_ae_entry(rvalue));
> -
> -   if (debug)
> -      dump_ae(ae);
> -}
> -
> -void
> -cse_visitor::handle_rvalue(ir_rvalue **rvalue)
> -{
> -   if (!*rvalue)
> -      return;
> -
> -   if (debug) {
> -      printf("CSE: handle_rvalue ");
> -      (*rvalue)->print();
> -      printf("\n");
> -   }
> -
> -   if (!is_cse_candidate(*rvalue))
> -      return;
> -
> -   ir_rvalue *new_rvalue = try_cse(*rvalue);
> -   if (new_rvalue) {
> -      *rvalue = new_rvalue;
> -      progress = true;
> -
> -      if (debug)
> -         validate_ir_tree(validate_instructions);
> -   } else {
> -      add_to_ae(rvalue);
> -   }
> -}
> -
> -ir_visitor_status
> -cse_visitor::visit_enter(ir_if *ir)
> -{
> -   handle_rvalue(&ir->condition);
> -
> -   empty_ae_list();
> -   visit_list_elements(this, &ir->then_instructions);
> -
> -   empty_ae_list();
> -   visit_list_elements(this, &ir->else_instructions);
> -
> -   empty_ae_list();
> -   return visit_continue_with_parent;
> -}
> -
> -ir_visitor_status
> -cse_visitor::visit_enter(ir_function_signature *ir)
> -{
> -   empty_ae_list();
> -   visit_list_elements(this, &ir->body);
> -
> -   empty_ae_list();
> -   return visit_continue_with_parent;
> -}
> -
> -ir_visitor_status
> -cse_visitor::visit_enter(ir_loop *ir)
> -{
> -   empty_ae_list();
> -   visit_list_elements(this, &ir->body_instructions);
> -
> -   empty_ae_list();
> -   return visit_continue_with_parent;
> -}
> -
> -ir_visitor_status
> -cse_visitor::visit_enter(ir_call *)
> -{
> -   /* Because call is an exec_list of ir_rvalues, handle_rvalue gets passed a
> -    * pointer to the (ir_rvalue *) on the stack.  Since we save those pointers
> -    * in the AE list, we can't let handle_rvalue get called.
> -    */
> -   return visit_continue_with_parent;
> -}
> -
> -/**
> - * Does a (uniform-value) constant subexpression elimination pass on the code
> - * present in the instruction stream.
> - */
> -bool
> -do_cse(exec_list *instructions)
> -{
> -   cse_visitor v(instructions);
> -
> -   visit_list_elements(&v, instructions);
> -
> -   return v.progress;
> -}
> --
> 2.4.9
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list