[Mesa-dev] [PATCH 2/2] glsl: Implement a lowering pass for gl_ClipDistance.

[Paul Berry]

On 19 September 2011 11:10, Ian Romanick <idr at freedesktop.org> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 09/15/2011 04:40 PM, Paul Berry wrote:
> > In i965 GEN6+ (and I suspect most other hardware), gl_ClipDistance
> > needs to be laid out as a pair of vec4's (the first containing
> > clip distances 0-3, and the second containing clip distances 4-7).
> > However, it is declared in GLSL as an array of 8 floats.
> >
> > This lowering pass acts at the GLSL level, modifying the
> > declaration of gl_ClipDistance so that it is an array of vec4's
> > rather than an array of floats, and renaming it to
> > gl_ClipDistanceMESA.  In addition, it modifies all accesses to the
> > array so that they access the appropiate component of one of the
> > vec4's.
> >
> > Since some hardware may not internally represent gl_ClipDistance as
> > a pair of vec4's, this lowering pass is optional.  To enable it,
> > set the LowerClipDistance flag in gl_shader_compiler_options to
> > true. --- src/glsl/Makefile                |    1 +
> > src/glsl/SConscript              |    1 +
> > src/glsl/ir_optimization.h       |    1 + src/glsl/linker.cpp
> > |    3 + src/glsl/lower_clip_distance.cpp |  347
> > ++++++++++++++++++++++++++++++++++++++ src/mesa/main/mtypes.h
> > |    1 + 6 files changed, 354 insertions(+), 0 deletions(-) create
> > mode 100644 src/glsl/lower_clip_distance.cpp
> >
> > diff --git a/src/glsl/Makefile b/src/glsl/Makefile index
> > c20a6c9..bee21c2 100644 --- a/src/glsl/Makefile +++
> > b/src/glsl/Makefile @@ -56,6 +56,7 @@ CXX_SOURCES = \
> > loop_analysis.cpp \ loop_controls.cpp \ loop_unroll.cpp \ +
> > lower_clip_distance.cpp \ lower_discard.cpp \
> > lower_if_to_cond_assign.cpp \ lower_instructions.cpp \ diff --git
> > a/src/glsl/SConscript b/src/glsl/SConscript index 1da58a9..b4786c5
> > 100644 --- a/src/glsl/SConscript +++ b/src/glsl/SConscript @@ -67,6
> > +67,7 @@ glsl_sources = [ 'loop_analysis.cpp',
> > 'loop_controls.cpp', 'loop_unroll.cpp', +
> > 'lower_clip_distance.cpp', 'lower_discard.cpp',
> > 'lower_if_to_cond_assign.cpp', 'lower_instructions.cpp', diff --git
> > a/src/glsl/ir_optimization.h b/src/glsl/ir_optimization.h index
> > 48448d4..af80e26 100644 --- a/src/glsl/ir_optimization.h +++
> > b/src/glsl/ir_optimization.h @@ -69,6 +69,7 @@ bool
> > lower_noise(exec_list *instructions); bool
> > lower_variable_index_to_cond_assign(exec_list *instructions, bool
> > lower_input, bool lower_output, bool lower_temp, bool
> > lower_uniform); bool lower_quadop_vector(exec_list *instructions,
> > bool dont_lower_swz); +bool lower_clip_distance(exec_list
> > *instructions); bool optimize_redundant_jumps(exec_list
> > *instructions);
> >
> > ir_rvalue * diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > index 195f58f..ceb49fd 100644 --- a/src/glsl/linker.cpp +++
> > b/src/glsl/linker.cpp @@ -1743,6 +1743,9 @@ link_shaders(struct
> > gl_context *ctx, struct gl_shader_program *prog) if
> > (!prog->LinkStatus) goto done;
> >
> > +      if (ctx->ShaderCompilerOptions[i].LowerClipDistance) +
> > lower_clip_distance(prog->_LinkedShaders[i]->ir); + while
> > (do_common_optimization(prog->_LinkedShaders[i]->ir, true, 32)) ;
> > } diff --git a/src/glsl/lower_clip_distance.cpp
> > b/src/glsl/lower_clip_distance.cpp new file mode 100644 index
> > 0000000..55a9e9d --- /dev/null +++
> > b/src/glsl/lower_clip_distance.cpp @@ -0,0 +1,347 @@ +/* + *
> > Copyright © 2011 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 lower_clip_distance.cpp + * + * This pass accounts
> > for the difference between the way + * gl_ClipDistance is declared
> > in standard GLSL (as an array of + * floats), and the way it is
> > frequently implemented in hardware (as + * a pair of vec4s, with
> > four clip distances packed into each). + * + * The declaration of
> > gl_ClipDistance is replaced with a declaration + * of
> > gl_ClipDistanceMESA, and any references to gl_ClipDistance are + *
> > translated to refer to gl_ClipDistanceMESA with the appropriate + *
> > swizzling of array indices.  For instance: + * + *
> > gl_ClipDistance[i] + * + * is translated into: + * + *
> > gl_ClipDistanceMESA[i>>2][i&3] + * + * Since some hardware may not
> > internally represent gl_ClipDistance as a pair + * of vec4's, this
> > lowering pass is optional.  To enable it, set the + *
> > LowerClipDistance flag in gl_shader_compiler_options to true. + */
> > + +#include "ir_hierarchical_visitor.h" +#include "ir.h" + +class
> > lower_clip_distance_visitor : public ir_hierarchical_visitor {
> > +public: +   lower_clip_distance_visitor() +      :
> > progress(false), old_clip_distance_var(NULL), +
> > new_clip_distance_var(NULL) +   { +   } + +   virtual
> > ir_visitor_status visit(ir_variable *); +   void
> > create_indices(ir_rvalue*, ir_rvalue *&, ir_rvalue *&); +   virtual
> > ir_visitor_status visit_leave(ir_dereference_array *); +   virtual
> > ir_visitor_status visit_leave(ir_assignment *); +   void
> > visit_new_assignment(ir_assignment *ir); +   virtual
> > ir_visitor_status visit_leave(ir_call *); + +   bool progress; + +
> > /** +    * Pointer to the declaration of gl_ClipDistance, if
> > found. +    */ +   ir_variable *old_clip_distance_var; + +   /** +
> > * Pointer to the newly-created gl_ClipDistanceMESA variable. +
> > */ +   ir_variable *new_clip_distance_var; +}; + + +/** + * Replace
> > any declaration of gl_ClipDistance as an array of floats with a + *
> > declaration of gl_ClipDistanceMESA as an array of vec4's. + */
> > +ir_visitor_status +lower_clip_distance_visitor::visit(ir_variable
> > *ir) +{ +   /* No point in looking for the declaration of
> > gl_ClipDistance if +    * we've already found it. +    */ +   if
> > (this->old_clip_distance_var) +      return visit_continue; + +
> > if (ir->name && strcmp(ir->name, "gl_ClipDistance") == 0) { +
> > this->progress = true; +      this->old_clip_distance_var = ir; +
> > assert (ir->type->is_array()); +      assert
> > (ir->type->element_type() == glsl_type::float_type); +
> > unsigned new_size = (ir->type->array_size() + 3) / 4; + +      /*
> > Clone the old var so that we inherit all of its properties */ +
> > this->new_clip_distance_var = ir->clone(ralloc_parent(ir), NULL);
> > + +      /* And change the properties that we need to change */ +
> > this->new_clip_distance_var->name +         =
> > ralloc_strdup(this->new_clip_distance_var, "gl_ClipDistanceMESA");
> > +      this->new_clip_distance_var->type +         =
> > glsl_type::get_array_instance(glsl_type::vec4_type, new_size); +
> > this->new_clip_distance_var->max_array_access =
> > ir->max_array_access / 4; + +
> > ir->replace_with(this->new_clip_distance_var); +   } +   return
> > visit_continue; +} + + +/** + * Create the necessary GLSL rvalues
> > to index into gl_ClipDistanceMESA based + * on the rvalue
> > previously used to index into gl_ClipDistance.  The first, + *
> > "outer" index, selects one of the vec4's in gl_ClipDistanceMESA,
> > and the + * second, "inner" index, selects a component within that
> > vec4. + */
>
> I'd call these array_index and swizzle_index instead of outer_index
> and inner_index.  Also, documenting function parameters should be done
> in the Doxygen magic format:
>
> /**
>  *
>  * \param array_index   Selects one of the vec4's in gl_ClipDistanceMESA
>  * \param swizzle_index Selects a component within the vec4 selected by
>  *                      array_index.
>  */
>

Ok, sounds reasonable.


>
> > +void +lower_clip_distance_visitor::create_indices(ir_rvalue
> > *old_index, +                                            ir_rvalue
> > *&outer_index, +
> > ir_rvalue *&inner_index) +{ +   void *ctx =
> > ralloc_parent(old_index); + +   /* Make sure old_index is a signed
> > int so that the bitwise "shift" and +    * "and" operations below
> > type check properly. +    */ +   if (old_index->type !=
> > glsl_type::int_type) { +      assert (old_index->type ==
> > glsl_type::uint_type); +      old_index = new(ctx)
> > ir_expression(ir_unop_u2i, old_index); +   }
>
> Does this matter?  The only case where it would be different is if the
> index is negative.  In that case, the array look-up already has
> undefined results.
>

The reason for this is that according to GLSL IR type validation rules, the
two operands to a binary AND need to be the same type, so in addition to
using old_index->type for old_index_var (as you mention below), I would have
to dynamically choose between ir_constant(3) and ir_constant(3u) based on
the type of the index.  Also, the call to get_int_component() would feel
more fragile, since technically one *ought* to use get_uint_component() to
get the value of a uint (even though get_int_component() works in practice
due to int and uint constants being stored in the same union).  Finally,
there are bugs in lower_vec_index_to_swizzle.cpp and
lower_variable_index_to_cond_assign.cpp that prevent them from working on
uint array indices right now.  Those should be pretty easy to fix but I
would rather get gl_ClipDistance to work first.

Considering that on most back-ends, converting an int to a uint should not
cost any GPU instructions (since it's just a reinterpretation of bits), just
doing the conversion seemed easier than dealing with all the stuff above.


>
> > + +   ir_constant *old_index_constant =
> > old_index->constant_expression_value(); +   if (old_index_constant)
> > { +      /* gl_ClipDistance is being accessed via a constant index.
> > Don't bother +       * creating expressions to calculate the
> > lowered indices.  Just create +       * constants. +       */ +
> > int const_val = old_index_constant->get_int_component(0); +
> > outer_index = new(ctx) ir_constant(const_val / 4); +
> > inner_index = new(ctx) ir_constant(const_val % 4); +   } else { +
> > /* Create a variable to hold the value of old_index (so that we +
> > * don't compute it twice). +       */ +      ir_variable
> > *old_index_var = new(ctx) ir_variable( +
> > glsl_type::int_type, "clip_distance_index", ir_var_temporary);
>
> Based on my comment above, I think this should use old_index->type
> instead of hard-coding glsl_type::int_type.
>
> > +      this->base_ir->insert_before(old_index_var); +
> > this->base_ir->insert_before(new(ctx) ir_assignment( +
> > new(ctx) ir_dereference_variable(old_index_var), old_index)); + +
> > /* Create the expression clip_distance_index / 4.  Do this as a
> > bit +       * shift because that's likely to be more efficient. +
> > */ +      outer_index = new(ctx) ir_expression( +
> > ir_binop_rshift, new(ctx) ir_dereference_variable(old_index_var), +
> > new(ctx) ir_constant(2)); + +      /* Create the expression
> > clip_distance_index % 4.  Do this as a bitwise +       * AND
> > because that's likely to be more efficient. +       */ +
> > inner_index = new(ctx) ir_expression( +         ir_binop_bit_and,
> > new(ctx) ir_dereference_variable(old_index_var), +         new(ctx)
> > ir_constant(3)); +   } +} + + +/** + * Replace any expression that
> > indexes into the gl_ClipDistance array with an + * expression that
> > indexes into one of the vec4's in gl_ClipDistanceMESA and + *
> > accesses the appropriate component. + */ +ir_visitor_status
> > +lower_clip_distance_visitor::visit_leave(ir_dereference_array
> > *ir) +{ +   /* If the gl_ClipDistance var hasn't been declared yet,
> > then +    * there's no way this deref can refer to it. +    */ +
> > if (!this->old_clip_distance_var) +      return visit_continue; + +
> > ir_dereference_variable *old_var_ref =
> > ir->array->as_dereference_variable(); +   if (old_var_ref &&
> > old_var_ref->variable_referenced() +       ==
> > this->old_clip_distance_var) { +      this->progress = true; +
> > ir_rvalue *outer_index; +      ir_rvalue *inner_index; +
> > this->create_indices(ir->array_index, outer_index, inner_index); +
> > void *mem_ctx = ralloc_parent(ir); +      ir->array = new(mem_ctx)
> > ir_dereference_array( +         this->new_clip_distance_var,
> > outer_index); +      ir->array_index = inner_index; +   } + +
> > return visit_continue; +} + + +/** + * Replace any assignment
> > having gl_ClipDistance (undereferenced) as its LHS + * or RHS with
> > a sequence of assignments, one for each component of the array. + *
> > Each of these assignments is lowered to refer to
> > gl_ClipDistanceMESA as + * appropriate. + */
>
> Don't we already have a lowering pass that does this?  If not, it
> seems like this could / should be it's own pass (that operates on
> general arrays).  Perhaps Eric will have an opinion...
>

I'm not aware of such a pass at the GLSL level (I think we may have
something like this in i965, but that wouldn't be useful here).  Eric, are
you aware of anything?


>
> > +ir_visitor_status
> > +lower_clip_distance_visitor::visit_leave(ir_assignment *ir) +{ +
> > ir_dereference_variable *lhs_var =
> > ir->lhs->as_dereference_variable(); +   ir_dereference_variable
> > *rhs_var = ir->rhs->as_dereference_variable(); +   if ((lhs_var &&
> > lhs_var->variable_referenced() +        ==
> > this->old_clip_distance_var) +       || (rhs_var &&
> > rhs_var->variable_referenced() +           ==
> > this->old_clip_distance_var)) { +      /* LHS or RHS of the
> > assignment is the entire gl_ClipDistance array. +       * Since we
> > are reshaping gl_ClipDistance from an array of floats to an +
> > * array of vec4's, this isn't going to work as a bulk assignment +
> > * anymore, so unroll it to element-by-element assignments and
> > lower +       * each of them. +       * +       * Note: to unroll
> > into element-by-element assignments, we need to make +       *
> > clones of the LHS and RHS.  This is only safe if the LHS and RHS
> > are +       * side-effect free.  Fortunately, we know that they
> > are, because the +       * only kind of rvalue that can have side
> > effects is an ir_call, and +       * ir_calls only appear (a) as a
> > statement on their own, or (b) as the +       * RHS of an
> > assignment that stores the result of the call in a +       *
> > temporary variable. +       */ +      void *ctx =
> > ralloc_parent(ir); +      int array_size =
> > this->old_clip_distance_var->type->array_size(); +      for (int i
> > = 0; i < array_size; ++i) { +         ir_dereference_array *new_lhs
> > = new(ctx) ir_dereference_array( +            ir->lhs->clone(ctx,
> > NULL), new(ctx) ir_constant(i)); +
> > this->visit_leave(new_lhs); +         ir_dereference_array *new_rhs
> > = new(ctx) ir_dereference_array( +            ir->rhs->clone(ctx,
> > NULL), new(ctx) ir_constant(i)); +
> > this->visit_leave(new_rhs); +
> > this->base_ir->insert_before( +            new(ctx)
> > ir_assignment(new_lhs, new_rhs)); +      } +      ir->remove(); +
> > } + +   return visit_continue; +} + + +/** + * Set up base_ir
> > properly and call visit_leave() on a newly created + *
> > ir_assignment node.  This is used in cases where we have to insert
> > an + * ir_assignment in a place where we know the hierarchical
> > visitor won't see + * it. + */ +void
> > +lower_clip_distance_visitor::visit_new_assignment(ir_assignment
> > *ir) +{ +   ir_instruction *old_base_ir = this->base_ir; +
> > this->base_ir = ir; +   this->visit_leave(ir); +   this->base_ir =
> > old_base_ir; +} + + +/** + * If gl_ClipDistance appears as an
> > argument in an ir_call expression, replace + * it with a temporary
> > variable, and make sure the ir_call is preceded and/or + * followed
> > by assignments that copy the contents of the temporary variable to
> > + * and/or from gl_ClipDistance.  Each of these assignments is then
> > lowered to + * refer to gl_ClipDistanceMESA. + */
> > +ir_visitor_status
> > +lower_clip_distance_visitor::visit_leave(ir_call *ir) +{ +   void
> > *ctx = ralloc_parent(ir); + +   const exec_node *formal_param_node
> > = ir->get_callee()->parameters.head; +   const exec_node
> > *actual_param_node = ir->actual_parameters.head; +   while
> > (!actual_param_node->is_tail_sentinel()) { +      ir_variable
> > *formal_param = (ir_variable *) formal_param_node; +      ir_rvalue
> > *actual_param = (ir_rvalue *) actual_param_node; + +      /*
> > Advance formal_param_node and actual_param_node now so that we can
> > +       * safely replace actual_param with another node, if
> > necessary, below. +       */ +      formal_param_node =
> > formal_param_node->next; +      actual_param_node =
> > actual_param_node->next; + +      ir_dereference_variable *deref =
> > actual_param->as_dereference_variable(); +      if (deref &&
> > deref->variable_referenced() +          ==
> > this->old_clip_distance_var) { +         /* User is trying to pass
> > gl_ClipDistance as an in, out, or inout +          * parameter to a
> > function call.  Since we are reshaping +          * gl_ClipDistance
> > from an array of floats to an array of vec4's, +          * this
> > isn't going to work anymore, so use a temporary array +          *
> > instead. +          */ +         ir_variable *temp_clip_distance =
> > new(ctx) ir_variable( +            actual_param->type,
> > "temp_clip_distance", ir_var_temporary); +
> > this->base_ir->insert_before(temp_clip_distance); +
> > actual_param->replace_with( +            new(ctx)
> > ir_dereference_variable(temp_clip_distance)); +         if
> > (formal_param->mode == ir_var_in +             ||
> > formal_param->mode == ir_var_inout) { +            /* Copy from
> > gl_ClipDistance to the temporary before the call. +             *
> > Since we are going to insert this copy before the current +
> > * instruction, we need to visit it afterwards to make sure it +
> > * gets lowered. +             */ +            ir_assignment
> > *new_assignment = new(ctx) ir_assignment( +               new(ctx)
> > ir_dereference_variable(temp_clip_distance), +
> > actual_param->clone(ctx, NULL)); +
> > this->base_ir->insert_before(new_assignment); +
> > this->visit_new_assignment(new_assignment); +         } +
> > if (formal_param->mode == ir_var_out +             ||
> > formal_param->mode == ir_var_inout) { +            /* Copy from the
> > temporary to gl_ClipDistance after the call. +             * Since
> > visit_list_elements() has already decided which +             *
> > instruction it's going to visit next, we need to visit +
> > * afterwards to make sure it gets lowered. +             */ +
> > ir_assignment *new_assignment = new(ctx) ir_assignment( +
> > actual_param->clone(ctx, NULL), +               new(ctx)
> > ir_dereference_variable(temp_clip_distance)); +
> > this->base_ir->insert_after(new_assignment); +
> > this->visit_new_assignment(new_assignment); +         } +      } +
> > } + +   return visit_continue; +} + + +bool
> > +lower_clip_distance(exec_list *instructions) +{ +
> > lower_clip_distance_visitor v; + +   visit_list_elements(&v,
> > instructions); + +   return v.progress; +} diff --git
> > a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h index
> > 8671ecd..ef04749 100644 --- a/src/mesa/main/mtypes.h +++
> > b/src/mesa/main/mtypes.h @@ -2297,6 +2297,7 @@ struct
> > gl_shader_compiler_options GLboolean EmitNoMainReturn;
> > /**< Emit CONT/RET opcodes? */ GLboolean EmitNoNoise;
> > /**< Emit NOISE opcodes? */ GLboolean EmitNoPow;
> > /**< Emit POW opcodes? */ +   GLboolean LowerClipDistance; /**<
> > Lower gl_ClipDistance from float[8] to vec4[2]? */
> >
> > /** * \name Forms of indirect addressing the driver cannot do.
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAk53hZUACgkQX1gOwKyEAw/wVQCfbjFA83RlnUOSXd57EbPCGDQl
> WvYAnRyiiNA6JL1K6q3wKdLgkg/vyjFh
> =9UAG
> -----END PGP SIGNATURE-----
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110919/8e301330/attachment-0001.htm>