[Mesa-dev] [PATCH 2/2] glsl: Implement a lowering pass for gl_ClipDistance.
Eric Anholt
eric at anholt.net
Wed Sep 21 10:51:38 PDT 2011
On Mon, 19 Sep 2011 13:40:47 -0700, Paul Berry <stereotype441 at gmail.com> wrote:
Non-text part: multipart/alternative
> 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?
I think he's thinking of brw_fs_vector_splitting? But it's not general
like that, and is also horrible awful fragile code.
We do need something like opt_structure_splitting for arrays, which is
kind of a similar idea but I don't imagine you could get much use out of
sharing code (since that would want to split it up into separate
variables, not accesses to one variable).
What I really wish was that we broke down aggregate assignments at the
AST to HIR level so that I never had to think about them in
transformations or codegen (except for function inlining). I wonder how
possible that would be?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110921/8c1bb4a4/attachment.pgp>
More information about the mesa-dev
mailing list