[Mesa-dev] [PATCH 7/8] glsl: Optimize "if (cond) discard; " to a conditional discard.

[Ian Romanick]

On 02/24/2015 02:19 AM, Kenneth Graunke wrote:
> st_glsl_to_tgsi and ir_to_mesa have handled conditional discards for a
> long time; the previous patch added that capability to i965.
> 
> i965 (Haswell) shader-db stats:
> 
> Without NIR:
> total instructions in shared programs: 5792133 -> 5776360 (-0.27%)
> instructions in affected programs:     737585 -> 721812 (-2.14%)
> helped:                                6300
> HURT:                                  68
> GAINED:                                2
> 
> With NIR:
> total instructions in shared programs: 5787538 -> 5769569 (-0.31%)
> instructions in affected programs:     767843 -> 749874 (-2.34%)
> helped:                                6522
> HURT:                                  35
> GAINED:                                6
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/glsl/Makefile.sources            |  1 +
>  src/glsl/glsl_parser_extras.cpp      |  1 +
>  src/glsl/ir_optimization.h           |  1 +
>  src/glsl/opt_conditional_discard.cpp | 81 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 84 insertions(+)
>  create mode 100644 src/glsl/opt_conditional_discard.cpp
> 
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index d0210d1..b876642 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -157,6 +157,7 @@ LIBGLSL_FILES = \
>  	lower_ubo_reference.cpp \
>  	opt_algebraic.cpp \
>  	opt_array_splitting.cpp \
> +	opt_conditional_discard.cpp \
>  	opt_constant_folding.cpp \
>  	opt_constant_propagation.cpp \
>  	opt_constant_variable.cpp \
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index 9f79313..f19804b 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -1627,6 +1627,7 @@ do_common_optimization(exec_list *ir, bool linked,
>     }
>     progress = do_if_simplification(ir) || progress;
>     progress = opt_flatten_nested_if_blocks(ir) || progress;
> +   progress = opt_conditional_discard(ir) || progress;
>     progress = do_copy_propagation(ir) || progress;
>     progress = do_copy_propagation_elements(ir) || progress;
>  
> diff --git a/src/glsl/ir_optimization.h b/src/glsl/ir_optimization.h
> index 7eb861a..e6939f3 100644
> --- a/src/glsl/ir_optimization.h
> +++ b/src/glsl/ir_optimization.h
> @@ -77,6 +77,7 @@ bool do_common_optimization(exec_list *ir, bool linked,
>  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/glsl/opt_conditional_discard.cpp b/src/glsl/opt_conditional_discard.cpp
> new file mode 100644
> index 0000000..0905188
> --- /dev/null
> +++ b/src/glsl/opt_conditional_discard.cpp
> @@ -0,0 +1,81 @@
> +/*
> + * Copyright © 2010 Intel Corporation

You've been sitting on this patch for a long time, eh? :)

> + *
> + * 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 "glsl_types.h"
> +#include "ir.h"
> +
> +namespace {
> +
> +class opt_conditional_discard_visitor : public ir_hierarchical_visitor {
> +public:
> +   opt_conditional_discard_visitor()
> +   {
> +      progress = false;

Did we ever decide whether it was better to initialize members like this
or like

   opt_conditional_discard_visitor()
      : progress(false)
   {
   }

I honestly don't remember.  I know we talked about it.

Either way, with the copyright date fixed, this patch is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> +   }
> +
> +   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.head->next->is_tail_sentinel() ||
> +       !((ir_instruction *) ir->then_instructions.head)->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.head;
> +   discard->condition = ir->condition;
> +   ir->replace_with(discard);
> +
> +   progress = true;
> +
> +   return visit_continue;
> +}
>