[Mesa-dev] [PATCH 11/34] glsl: support compilation of geometry shaders
Paul Berry
stereotype441 at gmail.com
Tue Jul 30 22:55:55 PDT 2013
On 30 July 2013 15:16, Kenneth Graunke <kenneth at 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?)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130730/4cca2489/attachment-0001.html>
More information about the mesa-dev
mailing list