[Mesa-dev] [PATCH 09/12] glsl: Add a pass to propagate the "invariant" and "precise" qualifiers
Francisco Jerez
currojerez at riseup.net
Sat Mar 19 05:00:09 UTC 2016
Jason Ekstrand <jason at jlekstrand.net> writes:
> On Fri, Mar 18, 2016 at 2:32 PM, Francisco Jerez <currojerez at riseup.net>
> wrote:
>
>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>
>> > ---
>> > src/compiler/Makefile.sources | 1 +
>> > src/compiler/glsl/glsl_parser_extras.cpp | 1 +
>> > src/compiler/glsl/ir_optimization.h | 1 +
>> > src/compiler/glsl/propagate_invariance.cpp | 125
>> +++++++++++++++++++++++++++++
>> > 4 files changed, 128 insertions(+)
>> > create mode 100644 src/compiler/glsl/propagate_invariance.cpp
>> >
>> > diff --git a/src/compiler/Makefile.sources
>> b/src/compiler/Makefile.sources
>> > index 9f3bcf0..6ab0aa7 100644
>> > --- a/src/compiler/Makefile.sources
>> > +++ b/src/compiler/Makefile.sources
>> > @@ -129,6 +129,7 @@ LIBGLSL_FILES = \
>> > glsl/opt_tree_grafting.cpp \
>> > glsl/opt_vectorize.cpp \
>> > glsl/program.h \
>> > + glsl/propagate_invariance.cpp \
>> > glsl/s_expression.cpp \
>> > glsl/s_expression.h
>> >
>> > diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
>> b/src/compiler/glsl/glsl_parser_extras.cpp
>> > index 1c6cd43..9fcca21 100644
>> > --- a/src/compiler/glsl/glsl_parser_extras.cpp
>> > +++ b/src/compiler/glsl/glsl_parser_extras.cpp
>> > @@ -1885,6 +1885,7 @@ do_common_optimization(exec_list *ir, bool linked,
>> > OPT(do_dead_functions, ir);
>> > OPT(do_structure_splitting, ir);
>> > }
>> > + propagate_invariance(ir);
>> > OPT(do_if_simplification, ir);
>> > OPT(opt_flatten_nested_if_blocks, ir);
>> > OPT(opt_conditional_discard, ir);
>> > diff --git a/src/compiler/glsl/ir_optimization.h
>> b/src/compiler/glsl/ir_optimization.h
>> > index b56413a..4616f16 100644
>> > --- a/src/compiler/glsl/ir_optimization.h
>> > +++ b/src/compiler/glsl/ir_optimization.h
>> > @@ -138,6 +138,7 @@ bool lower_tess_level(gl_shader *shader);
>> > bool lower_vertex_id(gl_shader *shader);
>> >
>> > bool lower_subroutine(exec_list *instructions, struct
>> _mesa_glsl_parse_state *state);
>> > +void propagate_invariance(exec_list *instructions);
>> >
>> > ir_rvalue *
>> > compare_index_block(exec_list *instructions, ir_variable *index,
>> > diff --git a/src/compiler/glsl/propagate_invariance.cpp
>> b/src/compiler/glsl/propagate_invariance.cpp
>> > new file mode 100644
>> > index 0000000..820259e
>> > --- /dev/null
>> > +++ b/src/compiler/glsl/propagate_invariance.cpp
>> > @@ -0,0 +1,125 @@
>> > +/*
>> > + * Copyright © 2010 Intel Corporation
>> > + *
>>
>> Wow, that's been a while ;)
>>
>
> Right. That's what I get for blindly copying and pasting. Updated to 2016
> locally. Thanks!
>
>
>> > + * 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 propagate_invariance.cpp
>> > + * Propagate the "invariant" and "precise" qualifiers to variables used
>> to
>> > + * compute invariant or precise values.
>> > + *
>> > + * The GLSL spec (depending on what version you read) says, among the
>> > + * conditions for geting bit-for-bit the same values on an invariant
>> output:
>> > + *
>> > + * "All operations in the consuming expressions and any intermediate
>> > + * expressions must be the same, with the same order of operands and
>> same
>> > + * associativity, to give the same order of evaluation."
>> > + *
>> > + * This effectively means that if a variable is used to compute an
>> invariant
>> > + * value then that variable becomes invariant. The same should apply
>> to the
>> > + * "precise" qualifier.
>> > + */
>> > +
>> > +#include "ir.h"
>> > +#include "ir_visitor.h"
>> > +#include "ir_rvalue_visitor.h"
>> > +#include "ir_optimization.h"
>> > +#include "compiler/glsl_types.h"
>> > +
>> > +namespace {
>> > +
>> > +class ir_invariance_propagation_visitor : public
>> ir_hierarchical_visitor {
>> > +public:
>> > + ir_invariance_propagation_visitor()
>> > + {
>> > + this->progress = false;
>> > + this->dst_var = NULL;
>> > + }
>> > +
>> > + virtual ~ir_invariance_propagation_visitor()
>> > + {
>> > + /* empty */
>> > + }
>> > +
>>
>> Redundant destructor?
>>
>
> Perhaps? I thought it was somewhat customary to always provide a virtual
> destructor whenever you have any sort of "real" inheritance going on. I
> guess it's probably ok since the one instantiation of this class is in this
> file. However, the other passes I looked at all had destructors exactly
> like that one so I just followed what they did.
>
Heh, right, some people do that as rule of thumb, but it's pretty
useless if your class is not intended to be derived from (and because
you have defined it in a cpp source file inside an anonymous namespace
it's effectively final). If you were designing an interface or a class
intended to be derived from, it would definitely make sense, but in the
definition of a leaf class in the inheritance hierarchy you have two
possibilities: Either your base class already has a virtual destructor
in which case you're fine no matter what you do, or your base class
doesn't in which case you're screwed even if you make your destructor
virtual, because "delete p" for p a pointer to your base class will
still call the non-virtual destructor of the base class only -- Blame
ir_hierarchical_visitor for not providing a virtual destructor.
> I don't care one way or the other. I can remove it if you'd like.
I don't either, you can keep my R-b either way.
> --Jason
>
>
>> > + virtual ir_visitor_status visit_enter(ir_assignment *ir);
>> > + virtual ir_visitor_status visit_leave(ir_assignment *ir);
>> > + virtual ir_visitor_status visit(ir_dereference_variable *ir);
>> > +
>> > + ir_variable *dst_var;
>> > + bool progress;
>> > +};
>> > +
>> > +} /* unnamed namespace */
>> > +
>> > +ir_visitor_status
>> > +ir_invariance_propagation_visitor::visit_enter(ir_assignment *ir)
>> > +{
>> > + assert(this->dst_var == NULL);
>> > + ir_variable *var = ir->lhs->variable_referenced();
>> > + if (var->data.invariant || var->data.precise) {
>> > + this->dst_var = var;
>> > + return visit_continue;
>> > + } else {
>> > + return visit_continue_with_parent;
>> > + }
>> > +}
>> > +
>> > +ir_visitor_status
>> > +ir_invariance_propagation_visitor::visit_leave(ir_assignment *ir)
>> > +{
>> > + this->dst_var = NULL;
>> > +
>> > + return visit_continue;
>> > +}
>> > +
>> > +ir_visitor_status
>> > +ir_invariance_propagation_visitor::visit(ir_dereference_variable *ir)
>> > +{
>> > + if (this->dst_var == NULL)
>> > + return visit_continue;
>> > +
>> > + if (this->dst_var->data.invariant) {
>> > + if (!ir->var->data.invariant)
>> > + this->progress = true;
>> > +
>> > + ir->var->data.invariant = true;
>> > + }
>> > +
>> > + if (this->dst_var->data.precise) {
>> > + if (!ir->var->data.precise)
>> > + this->progress = true;
>> > +
>> > + ir->var->data.precise = true;
>> > + }
>> > +
>> > + return visit_continue;
>> > +}
>> > +
>> > +void
>> > +propagate_invariance(exec_list *instructions)
>> > +{
>> > + ir_invariance_propagation_visitor visitor;
>> > +
>> > + do {
>> > + visitor.progress = false;
>> > + visit_list_elements(&visitor, instructions);
>> > + } while (visitor.progress);
>> > +}
>>
>> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>>
>> > --
>> > 2.5.0.400.gff86faf
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160318/961ab087/attachment.sig>
More information about the mesa-dev
mailing list