[Mesa-dev] [PATCH 3/4] glsl: Merge if-simplification and conditional discard optimization

[Matt Turner]

On Thu, Apr 6, 2017 at 12:49 PM, Thomas Helland
<thomashelland90 at gmail.com> wrote:
> The conditional discard pass follows the same pattern, so merge the
> two, and avoid running the visitor two times.
> ---
>  src/compiler/Makefile.sources                 |  1 -
>  src/compiler/glsl/glsl_parser_extras.cpp      |  1 -
>  src/compiler/glsl/ir_optimization.h           |  1 -
>  src/compiler/glsl/opt_conditional_discard.cpp | 88 ---------------------------
>  src/compiler/glsl/opt_if_simplification.cpp   | 50 +++++++++++----
>  5 files changed, 39 insertions(+), 102 deletions(-)
>  delete mode 100644 src/compiler/glsl/opt_conditional_discard.cpp
>
> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> index 5197c31bb3..817ab3a755 100644
> --- a/src/compiler/Makefile.sources
> +++ b/src/compiler/Makefile.sources
> @@ -114,7 +114,6 @@ LIBGLSL_FILES = \
>         glsl/lower_ubo_reference.cpp \
>         glsl/opt_algebraic.cpp \
>         glsl/opt_array_splitting.cpp \
> -       glsl/opt_conditional_discard.cpp \
>         glsl/opt_constant_folding.cpp \
>         glsl/opt_constant_propagation.cpp \
>         glsl/opt_constant_variable.cpp \
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp b/src/compiler/glsl/glsl_parser_extras.cpp
> index 307e0d6215..fe25c23ccc 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -2139,7 +2139,6 @@ do_common_optimization(exec_list *ir, bool linked,
>     }
>     propagate_invariance(ir);
>     OPT(do_if_simplification, ir);
> -   OPT(opt_conditional_discard, ir);
>     OPT(do_copy_propagation, ir);
>     OPT(do_copy_propagation_elements, ir);
>
> diff --git a/src/compiler/glsl/ir_optimization.h b/src/compiler/glsl/ir_optimization.h
> index 5d57ca85fd..022b7ab12f 100644
> --- a/src/compiler/glsl/ir_optimization.h
> +++ b/src/compiler/glsl/ir_optimization.h
> @@ -93,7 +93,6 @@ bool ir_constant_fold(ir_rvalue **rvalue);
>  bool do_rebalance_tree(exec_list *instructions);
>  bool do_algebraic(exec_list *instructions, bool native_integers,
>                    const struct gl_shader_compiler_options *options);
> -bool opt_conditional_discard(exec_list *instructions);
>  bool do_constant_folding(exec_list *instructions);
>  bool do_constant_variable(exec_list *instructions);
>  bool do_constant_variable_unlinked(exec_list *instructions);
> diff --git a/src/compiler/glsl/opt_conditional_discard.cpp b/src/compiler/glsl/opt_conditional_discard.cpp
> deleted file mode 100644
> index 6d8a23460d..0000000000
> --- a/src/compiler/glsl/opt_conditional_discard.cpp
> +++ /dev/null
> @@ -1,88 +0,0 @@
> -/*
> - * Copyright © 2014 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_conditional_discard.cpp
> - *
> - * Replace
> - *
> - *    if (cond) discard;
> - *
> - * with
> - *
> - *    (discard <condition>)
> - */
> -
> -#include "compiler/glsl_types.h"
> -#include "ir.h"
> -
> -namespace {
> -
> -class opt_conditional_discard_visitor : public ir_hierarchical_visitor {
> -public:
> -   opt_conditional_discard_visitor()
> -   {
> -      progress = false;
> -   }
> -
> -   ir_visitor_status visit_leave(ir_if *);
> -
> -   bool progress;
> -};
> -
> -} /* anonymous namespace */
> -
> -bool
> -opt_conditional_discard(exec_list *instructions)
> -{
> -   opt_conditional_discard_visitor v;
> -   v.run(instructions);
> -   return v.progress;
> -}
> -
> -ir_visitor_status
> -opt_conditional_discard_visitor::visit_leave(ir_if *ir)
> -{
> -   /* Look for "if (...) discard" with no else clause or extra statements. */
> -   if (ir->then_instructions.is_empty() ||
> -       !ir->then_instructions.get_head_raw()->next->is_tail_sentinel() ||
> -       !((ir_instruction *) ir->then_instructions.get_head_raw())->as_discard() ||
> -       !ir->else_instructions.is_empty())
> -      return visit_continue;
> -
> -   /* Move the condition and replace the ir_if with the ir_discard. */
> -   ir_discard *discard = (ir_discard *) ir->then_instructions.get_head_raw();
> -   if (!discard->condition)
> -      discard->condition = ir->condition;
> -   else {
> -      void *ctx = ralloc_parent(ir);
> -      discard->condition = new(ctx) ir_expression(ir_binop_logic_and,
> -                                                  ir->condition,
> -                                                  discard->condition);
> -   }
> -   ir->replace_with(discard);
> -
> -   progress = true;
> -
> -   return visit_continue;
> -}
> diff --git a/src/compiler/glsl/opt_if_simplification.cpp b/src/compiler/glsl/opt_if_simplification.cpp
> index 05159319ba..64c56d0a2f 100644
> --- a/src/compiler/glsl/opt_if_simplification.cpp
> +++ b/src/compiler/glsl/opt_if_simplification.cpp
> @@ -44,23 +44,12 @@ public:
>     }
>
>     ir_visitor_status visit_leave(ir_if *);
> -   ir_visitor_status visit_enter(ir_assignment *);
>
>     bool made_progress;
>  };
>
>  } /* unnamed namespace */
>
> -/* We only care about the top level "if" instructions, so don't
> - * descend into expressions.
> - */
> -ir_visitor_status
> -ir_if_simplification_visitor::visit_enter(ir_assignment *ir)
> -{
> -   (void) ir;
> -   return visit_continue_with_parent;
> -}

I don't understand this change. What's going on?

(Everything else in the series looks good. I've made a few trivial
changes locally -- whitespace mostly)