On 19 September 2011 11:10, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 <div><div></div><div class="h5"> 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. + */ </div></div>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. <div class="im"> */ </div></blockquote><div> Ok, sounds reasonable. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="im"> > +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); + } </div>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. </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> > + + ir_constant *old_index_constant = <div class="im">> 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); </div>Based on my comment above, I think this should use old_index->type instead of hard-coding glsl_type::int_type. <div><div></div><div class="h5"> > + 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. + */ </div></div>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... </blockquote><div> 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? </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> <div><div></div><div class="h5"> > +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. </div></div>-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - <a href="http://enigmail.mozdev.org/" target="_blank">http://enigmail.mozdev.org/</a> iEYEARECAAYFAk53hZUACgkQX1gOwKyEAw/wVQCfbjFA83RlnUOSXd57EbPCGDQl WvYAnRyiiNA6JL1K6q3wKdLgkg/vyjFh =9UAG -----END PGP SIGNATURE----- </blockquote></div>