<div dir="ltr">On 1 August 2013 16:03, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> A couple of comments below. Other than that, Reviewed-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>><div><div class="h5"> On 07/31/2013 02:18 PM, Paul Berry wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- src/glsl/ir_set_program_inouts.cpp | 85 ++++++++++++++++++++++++++++++++------ 1 file changed, 73 insertions(+), 12 deletions(-) diff --git a/src/glsl/ir_set_program_inouts.cpp b/src/glsl/ir_set_program_inouts.cpp index 018342b..8f96324 100644 --- a/src/glsl/ir_set_program_inouts.cpp +++ b/src/glsl/ir_set_program_inouts.cpp @@ -114,7 +114,13 @@ mark(struct gl_program *prog, ir_variable *var, int offset, int len, void ir_set_program_inouts_visitor::mark_whole_variable(ir_variable *var) { - mark(this->prog, var, 0, var->type->count_attribute_slots(), + const glsl_type *type = var->type; + if (this->shader_type == GL_GEOMETRY_SHADER && + var->mode == ir_var_shader_in && type->is_array()) { + type = type->fields.array; + } + + mark(this->prog, var, 0, type->count_attribute_slots(), this->shader_type == GL_FRAGMENT_SHADER); } @@ -133,7 +139,9 @@ ir_set_program_inouts_visitor::visit(ir_dereference_variable *ir) /** * Try to mark a portion of the given variable as used. Caller must ensure * that the variable represents a shader input or output which can be indexed - * into in array fashion (an array, matrix, or vector). + * into in array fashion (an array, matrix, or vector). For the purpose of + * geometry shader inputs (which are always arrays), this means that the array + * element must be something that can be indexed into in array fashion. </blockquote> </div></div> Except gl_PrimitiveIDIn, as noted below.</blockquote><div> </div><div>Good point. I've updated the comment. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> * * If the index can't be interpreted as a constant, or some other problem * occurs, then nothing will be marked and false will be returned. @@ -144,6 +152,16 @@ ir_set_program_inouts_visitor::try_mark_partial_variable(ir_variable *var, { const glsl_type *type = var->type; + if (this->shader_type == GL_GEOMETRY_SHADER && + var->mode == ir_var_shader_in) { + /* The only geometry shader input that is not an array is + * gl_PrimitiveIDIn, and in that case, this code will never be reached, + * because gl_PrimitiveIDIn can't be indexed into in array fashion. + */ + assert(type->is_array()); + type = type->fields.array; + } + /* The code below only handles: * * - Indexing into matrices @@ -205,17 +223,60 @@ ir_set_program_inouts_visitor::try_mark_partial_variable(ir_variable *var, ir_visitor_status ir_set_program_inouts_visitor::visit_enter(ir_dereference_array *ir) { - ir_dereference_variable *deref_var; - deref_var = ir->array->as_dereference_variable(); - ir_variable *var = deref_var ? deref_var->var : NULL; - - /* Check that we're dereferencing a shader in or out */ - if (!var || !is_shader_inout(var)) - return visit_continue; - - if (try_mark_partial_variable(var, ir->array_index)) - return visit_continue_with_parent; + /* Note: for geometry shader inputs, lower_named_interface_blocks may + * create 2D arrays, so we need to be able to handle those. 2D arrays + * shouldn't be able to crop up for any other reason. + */ + if (ir_dereference_array *inner_array = + ir->array->as_dereference_array()) { </blockquote> </div></div> 'ir_dereference_array *const'? I'm not sure whether I love or hate this style. I seem some significant advantages to it (the big one being the limited scope of the variable), but it has some stylistic issues (the long if-condition). Having an assignment in an if-condition also feels icky. I guess it's pretty similar to what we already do with for-loops, so I'm leaning towards liking it.</blockquote><div> </div><div>Those are all fair points. I'm leaning towards liking it too, so I'm going to go ahead with it. If there are strong objections later, we can always change it. </div><div>I like the idea of marking these declarations-inside-if-statements as const--it reduces the potential ick a little bit by clarifying that we're not going to do anything dangerous with them :) Thanks again for all the code review on these two huge patch series. </div></div></div></div>