[Mesa-dev] [PATCH 2/2] glsl: merge loop_controls.cpp with loop_unroll.cpp

Thomas Helland thomashelland90 at gmail.com
Wed Sep 20 19:45:41 UTC 2017


I've only skimmed this, but it looks trivial and correct.
This patch series is:
Reviewed-by: Thomas Helland <thomashelland90 at gmail.com>

I like the idea of getting rid of some of the walks over the IR,
and have a set of patches sitting locally doing just that.
I think I got most of them reviewed, but then it stalled.
I've just applied for an account though, so then I won't
have to bother others to push my patches for me =)

2017-09-19 4:14 GMT+02:00 Timothy Arceri <tarceri at itsqueeze.com>:
> Having this separate just makes the code harder to follow, and
> requires an extra walk of the IR.
> ---
>  src/compiler/Makefile.sources            |   1 -
>  src/compiler/glsl/glsl_parser_extras.cpp |   1 -
>  src/compiler/glsl/loop_analysis.h        |  16 -----
>  src/compiler/glsl/loop_controls.cpp      | 108 -------------------------------
>  src/compiler/glsl/loop_unroll.cpp        |  36 ++++++++++-
>  5 files changed, 34 insertions(+), 128 deletions(-)
>  delete mode 100644 src/compiler/glsl/loop_controls.cpp
>
> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> index e03a3f8738c..7146d3db367 100644
> --- a/src/compiler/Makefile.sources
> +++ b/src/compiler/Makefile.sources
> @@ -80,7 +80,6 @@ LIBGLSL_FILES = \
>         glsl/list.h \
>         glsl/loop_analysis.cpp \
>         glsl/loop_analysis.h \
> -       glsl/loop_controls.cpp \
>         glsl/loop_unroll.cpp \
>         glsl/lower_blend_equation_advanced.cpp \
>         glsl/lower_buffer_access.cpp \
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp b/src/compiler/glsl/glsl_parser_extras.cpp
> index fc56f21a5f0..98151fdb4a4 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -2218,7 +2218,6 @@ do_common_optimization(exec_list *ir, bool linked,
>     if (options->MaxUnrollIterations) {
>        loop_state *ls = analyze_loop_variables(ir);
>        if (ls->loop_found) {
> -         OPT(set_loop_controls, ir, ls);
>           OPT(unroll_loops, ir, ls, options);
>        }
>        delete ls;
> diff --git a/src/compiler/glsl/loop_analysis.h b/src/compiler/glsl/loop_analysis.h
> index e2eff9dbaed..8f824046945 100644
> --- a/src/compiler/glsl/loop_analysis.h
> +++ b/src/compiler/glsl/loop_analysis.h
> @@ -35,22 +35,6 @@ extern class loop_state *
>  analyze_loop_variables(exec_list *instructions);
>
>
> -/**
> - * Fill in loop control fields
> - *
> - * Based on analysis of loop variables, this function tries to remove
> - * redundant sequences in the loop of the form
> - *
> - *  (if (expression bool ...) (break))
> - *
> - * For example, if it is provable that one loop exit condition will
> - * always be satisfied before another, the unnecessary exit condition will be
> - * removed.
> - */
> -extern bool
> -set_loop_controls(exec_list *instructions, loop_state *ls);
> -
> -
>  extern bool
>  unroll_loops(exec_list *instructions, loop_state *ls,
>               const struct gl_shader_compiler_options *options);
> diff --git a/src/compiler/glsl/loop_controls.cpp b/src/compiler/glsl/loop_controls.cpp
> deleted file mode 100644
> index ad4aa189411..00000000000
> --- a/src/compiler/glsl/loop_controls.cpp
> +++ /dev/null
> @@ -1,108 +0,0 @@
> -/*
> - * Copyright © 2010 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.
> - */
> -
> -#include <limits.h>
> -#include "main/compiler.h"
> -#include "compiler/glsl_types.h"
> -#include "loop_analysis.h"
> -#include "ir_hierarchical_visitor.h"
> -
> -
> -namespace {
> -
> -class loop_control_visitor : public ir_hierarchical_visitor {
> -public:
> -   loop_control_visitor(loop_state *state)
> -   {
> -      this->state = state;
> -      this->progress = false;
> -   }
> -
> -   virtual ir_visitor_status visit_leave(ir_loop *ir);
> -
> -   loop_state *state;
> -
> -   bool progress;
> -};
> -
> -} /* anonymous namespace */
> -
> -ir_visitor_status
> -loop_control_visitor::visit_leave(ir_loop *ir)
> -{
> -   loop_variable_state *const ls = this->state->get(ir);
> -
> -   /* If we've entered a loop that hasn't been analyzed, something really,
> -    * really bad has happened.
> -    */
> -   if (ls == NULL) {
> -      assert(ls != NULL);
> -      return visit_continue;
> -   }
> -
> -   if (ls->limiting_terminator != NULL) {
> -      /* If the limiting terminator has an iteration count of zero, then we've
> -       * proven that the loop cannot run, so delete it.
> -       */
> -      int iterations = ls->limiting_terminator->iterations;
> -      if (iterations == 0) {
> -         ir->remove();
> -         this->progress = true;
> -         return visit_continue;
> -      }
> -   }
> -
> -   /* Remove the conditional break statements associated with all terminators
> -    * that are associated with a fixed iteration count, except for the one
> -    * associated with the limiting terminator--that one needs to stay, since
> -    * it terminates the loop.  Exception: if the loop still has a normative
> -    * bound, then that terminates the loop, so we don't even need the limiting
> -    * terminator.
> -    */
> -   foreach_in_list(loop_terminator, t, &ls->terminators) {
> -      if (t->iterations < 0)
> -         continue;
> -
> -      if (t != ls->limiting_terminator) {
> -         t->ir->remove();
> -
> -         assert(ls->num_loop_jumps > 0);
> -         ls->num_loop_jumps--;
> -
> -         this->progress = true;
> -      }
> -   }
> -
> -   return visit_continue;
> -}
> -
> -
> -bool
> -set_loop_controls(exec_list *instructions, loop_state *ls)
> -{
> -   loop_control_visitor v(ls);
> -
> -   v.run(instructions);
> -
> -   return v.progress;
> -}
> diff --git a/src/compiler/glsl/loop_unroll.cpp b/src/compiler/glsl/loop_unroll.cpp
> index dbb3fa2fa5c..7eea439454b 100644
> --- a/src/compiler/glsl/loop_unroll.cpp
> +++ b/src/compiler/glsl/loop_unroll.cpp
> @@ -305,7 +305,6 @@ ir_visitor_status
>  loop_unroll_visitor::visit_leave(ir_loop *ir)
>  {
>     loop_variable_state *const ls = this->state->get(ir);
> -   int iterations;
>
>     /* If we've entered a loop that hasn't been analyzed, something really,
>      * really bad has happened.
> @@ -315,6 +314,39 @@ loop_unroll_visitor::visit_leave(ir_loop *ir)
>        return visit_continue;
>     }
>
> +   if (ls->limiting_terminator != NULL) {
> +      /* If the limiting terminator has an iteration count of zero, then we've
> +       * proven that the loop cannot run, so delete it.
> +       */
> +      int iterations = ls->limiting_terminator->iterations;
> +      if (iterations == 0) {
> +         ir->remove();
> +         this->progress = true;
> +         return visit_continue;
> +      }
> +   }
> +
> +   /* Remove the conditional break statements associated with all terminators
> +    * that are associated with a fixed iteration count, except for the one
> +    * associated with the limiting terminator--that one needs to stay, since
> +    * it terminates the loop.  Exception: if the loop still has a normative
> +    * bound, then that terminates the loop, so we don't even need the limiting
> +    * terminator.
> +    */
> +   foreach_in_list(loop_terminator, t, &ls->terminators) {
> +      if (t->iterations < 0)
> +         continue;
> +
> +      if (t != ls->limiting_terminator) {
> +         t->ir->remove();
> +
> +         assert(ls->num_loop_jumps > 0);
> +         ls->num_loop_jumps--;
> +
> +         this->progress = true;
> +      }
> +   }
> +
>     if (ls->limiting_terminator == NULL) {
>        ir_instruction *last_ir =
>           (ir_instruction *) ir->body_instructions.get_tail();
> @@ -343,7 +375,7 @@ loop_unroll_visitor::visit_leave(ir_loop *ir)
>        return visit_continue;
>     }
>
> -   iterations = ls->limiting_terminator->iterations;
> +   int iterations = ls->limiting_terminator->iterations;
>
>     const int max_iterations = options->MaxUnrollIterations;
>
> --
> 2.13.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list