On 30 July 2013 15:16, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On 07/28/2013 11:03 PM, Paul Berry wrote: > >> + /* The 'varying in' and 'varying out' qualifiers can only be used >> with >> + * ARB_geometry_shader4 and EXT_geometry_shader4, which we don't >> support >> + * yet. >> + */ >> + if (this->type->qualifier.flags.**q.varying) { >> + if (this->type->qualifier.flags.**q.in<http://qualifier.flags.q.in>) >> { >> + _mesa_glsl_error(& loc, state, >> + "`varying in' qualifier in declaration of " >> + "`%s' only valid for geometry shaders using " >> + "ARB_geometry_shader4 or >> EXT_geometry_shader4", >> + decl->identifier); >> + } >> + else if (this->type->qualifier.flags.**q.out) { >> > > Style-nit: > > } else if (this->type->qualifier.flagsq.**out) { > > (Mesa historically has used the two-line style in your patch, but has > largely moved away from that. In particular, the compiler has always used > "} else {".) > > Might be nice to kill the tabs too, but not a big deal either way. Fixed. > > > diff --git a/src/glsl/ir.h b/src/glsl/ir.h >> index ae79a39..af9d77e 100644 >> --- a/src/glsl/ir.h >> +++ b/src/glsl/ir.h >> @@ -2112,7 +2112,7 @@ ir_has_call(ir_instruction *ir); >> >> extern void >> do_set_program_inouts(exec_**list *instructions, struct gl_program >> *prog, >> - bool is_fragment_shader); >> + GLenum shader_type); >> > > This patch is getting pretty huge. It might be nice to do the > s/is_fragment_shader/shader_**type/ in a separate patch, since that's an > obvious hunk. That's a very good idea. I've split it out to a new patch immediately preceding this one. @@ -112,14 +111,24 @@ > ir_set_program_inouts_visitor::visit(ir_dereference_variable *ir) > return visit_continue; > > if (ir->type->is_array()) { > - mark(this->prog, ir->var, 0, > - ir->type->length * ir->type->fields.array->matrix_columns, > - this->is_fragment_shader); > + int matrix_columns = ir->type->fields.array->matrix_columns; > + int length = ir->type->length; > I was wondering if this was left over from the ARB_gs4 stuff. Apparently > it's for lowered arrays of instance blocks? Maybe a comment would be in > order. Good point. I'm not sure I spent enough time reviewing this part of the commit myself, nor testing it. Tomorrow I'll try to verify that this is necessary and correct. > > > @@ -129,7 +138,40 @@ >> ir_set_program_inouts_visitor::visit_enter(ir_dereference_array *ir) >> ir_dereference_variable *deref_var; >> ir_constant *index = ir->array_index->as_constant()**; >> deref_var = ir->array->as_dereference_**variable(); >> - ir_variable *var = deref_var ? deref_var->var : NULL; >> + ir_variable *var; >> + bool is_vert_array = false, is_2D_array = false; >> + >> + /* Check whether this dereference is of a GS input array. These are >> special >> + * because the array index refers to the index of an input vertex >> instead of >> + * the attribute index. The exceptions to this exception are 2D >> arrays >> + * such as gl_TexCoordIn. For these, there is a nested >> dereference_array, >> + * where the inner index specifies the vertex and the outer index >> specifies >> + * the attribute. To complicate things further, matrix columns are >> also >> + * accessed with dereference_array. So we have to correctly handle 1D >> + * arrays of non-matrices, 1D arrays of matrices, 2D arrays of >> non-matrices, >> + * and 2D arrays of matrices. >> + */ >> + if (this->shader_type == GL_GEOMETRY_SHADER) { >> + if (!deref_var) { >> + /* Either an outer (attribute) dereference of a 2D array or a >> column >> + * dereference of an array of matrices. */ >> > > Style-nit: */ goes on a separate line. Fixed. > > > + ir_dereference_array *inner_deref = ir->array->as_dereference_* >> *array(); >> + assert(inner_deref); >> + deref_var = inner_deref->array->as_**dereference_variable(); >> + is_2D_array = true; >> + } >> + >> + if (deref_var && deref_var->var->mode == ir_var_shader_in) { >> + if (ir->type->is_array()) >> + /* Inner (vertex) dereference of a 2D array */ >> + return visit_continue; >> + else >> + /* Dereference of a 1D (vertex) array */ >> + is_vert_array = true; >> + } >> + } >> > > I'm not terribly comfortable with this code, but I'm not sure what to > suggest instead. One concern I have is varying structs. > > As I understand it, if a VS outputs a struct, the corresponding GS input > would be an array of structs. Wouldn't those be accessed via (array_ref > (record_ref ...) ...)? > > The code above seems to assume that if it's not > (array_ref (var_ref ...)) > then it *must* be > (array_ref (array_ref ...) ...) > which seems wrong, or at least dubious. > > The "this must be a matrix" stuff concerns me too. Good point. I'll look at this in more detail too. > > > @@ -174,6 +177,77 @@ private: >> }; >> >> >> +class geom_array_resize_visitor : public ir_hierarchical_visitor { >> > > This looks like it might be the code to cross-validate > > layout(triangles) > in vec4 foo[3]; > in vec4 foo[2]; > > and such. But only in part? It might be nice to have that in a separate > patch as well...it's complicated enough to not be lumped in with basic > plumbing. Good point. This class (and the code that invokes it) belongs in patch 33 (glsl: Implement rules for geometry shader input sizes). I've moved it there. > > > @@ -1648,10 +1750,13 @@ link_shaders(struct gl_context *ctx, struct >> gl_shader_program *prog) >> unsigned num_vert_shaders = 0; >> struct gl_shader **frag_shader_list; >> unsigned num_frag_shaders = 0; >> + struct gl_shader **geom_shader_list; >> + unsigned num_geom_shaders = 0; >> >> vert_shader_list = (struct gl_shader **) >> - calloc(2 * prog->NumShaders, sizeof(struct gl_shader *)); >> + calloc(3 * prog->NumShaders, sizeof(struct gl_shader *)); >> frag_shader_list = &vert_shader_list[prog->**NumShaders]; >> + geom_shader_list = &vert_shader_list[prog->**NumShaders * 2]; >> > > This code is getting ugly. It looks like the idea is to allocate two (now > three) separate arrays, but somebody decided to be "clever" and batch them > into a single calloc/free big enough to hold all of them. > > I would much prefer to see: > > struct gl_shader **vert_shader_list = > calloc(prog->NumShaders, sizeof(struct gl_shader *)); > struct gl_shader **frag_shader_list = > calloc(prog->NumShaders, sizeof(struct gl_shader *)); > struct gl_shader **geom_shader_list = > calloc(prog->NumShaders, sizeof(struct gl_shader *)); > > and then later: > > free(vert_shader_list); > free(frag_shader_list); > free(geom_shader_list); > > Linking is complicated enough without accidental complexity. You're right. Making three separate allocations is much better. I've made a pre-patch that switches to two allocations (vert_shader_list and frag_shader_list), and then this patch changes to just add geom_shader_list in the obvious way. > > > @@ -1860,7 +1985,7 @@ link_shaders(struct gl_context *ctx, struct >> gl_shader_program *prog) >> * non-zero, but the program object has no vertex or geometry >> * shader; >> */ >> - if (first >= MESA_SHADER_FRAGMENT) { >> + if (first == MESA_SHADER_FRAGMENT) { >> > > This hunk is unnecessary (but harmless). Fair enough. I think I'd prefer to keep it anyhow, since I think the code is marginally less confusing with "==" (saves someone from wondering: could "first" ever be greater than MESA_SHADER_FRAGMENT? What would that even mean?)
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev