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

[Kenneth Graunke]

On Tuesday, February 24, 2015 11:33:30 AM Ian Romanick wrote:
> 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? :)

Haha, oops :)  Yeah, that should be 2014 (October 2014).  Still a while!

> > + *
> > + * 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.

For simple fields like this there was no real consensus.  I think the
final comment at the time was, "let's not mandate a style, and leave it
up to the artistic sense of the author".  I don't particularly care in
this instance.

> Either way, with the copyright date fixed, this patch is
> 
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150224/3ac321ef/attachment-0001.sig>