<div dir="ltr">On 30 July 2013 15:16, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div>On 07/28/2013 11:03 PM, Paul Berry wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ /* The 'varying in' and 'varying out' qualifiers can only be used with<br>
+ * ARB_geometry_shader4 and EXT_geometry_shader4, which we don't support<br>
+ * yet.<br>
+ */<br>
+ if (this->type->qualifier.flags.<u></u>q.varying) {<br>
+ if (this->type-><a href="http://qualifier.flags.q.in" target="_blank">qualifier.flags.<u></u>q.in</a>) {<br>
+ _mesa_glsl_error(& loc, state,<br>
+ "`varying in' qualifier in declaration of "<br>
+ "`%s' only valid for geometry shaders using "<br>
+ "ARB_geometry_shader4 or EXT_geometry_shader4",<br>
+ decl->identifier);<br>
+ }<br>
+ else if (this->type->qualifier.flags.<u></u>q.out) {<br>
</blockquote>
<br></div></div>
Style-nit:<br>
<br>
} else if (this->type->qualifier.flagsq.<u></u>out) {<br>
<br>
(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 {".)<br>
<br>
Might be nice to kill the tabs too, but not a big deal either way.</blockquote><div><br></div><div>Fixed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
diff --git a/src/glsl/ir.h b/src/glsl/ir.h<br>
index ae79a39..af9d77e 100644<br>
--- a/src/glsl/ir.h<br>
+++ b/src/glsl/ir.h<br>
@@ -2112,7 +2112,7 @@ ir_has_call(ir_instruction *ir);<br>
<br>
extern void<br>
do_set_program_inouts(exec_<u></u>list *instructions, struct gl_program *prog,<br>
- bool is_fragment_shader);<br>
+ GLenum shader_type);<br>
</blockquote>
<br></div></div>
This patch is getting pretty huge. It might be nice to do the s/is_fragment_shader/shader_<u></u>type/ in a separate patch, since that's an obvious hunk.</blockquote><div><br></div><div>That's a very good idea. I've split it out to a new patch immediately preceding this one.<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
@@ -112,14 +111,24 @@ ir_set_program_inouts_visitor::visit(ir_dereference_variable *ir)<br>
return visit_continue;<br>
<br>
if (ir->type->is_array()) {<br>
- mark(this->prog, ir->var, 0,<br>
- ir->type->length * ir->type->fields.array->matrix_columns,<br>
- this->is_fragment_shader);<br>
+ int matrix_columns = ir->type->fields.array->matrix_columns;<br>
+ int length = ir->type->length;<br>
</blockquote>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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.</blockquote><div><br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
@@ -129,7 +138,40 @@ ir_set_program_inouts_visitor::visit_enter(ir_dereference_array *ir)<br>
ir_dereference_variable *deref_var;<br>
ir_constant *index = ir->array_index->as_constant()<u></u>;<br>
deref_var = ir->array->as_dereference_<u></u>variable();<br>
- ir_variable *var = deref_var ? deref_var->var : NULL;<br>
+ ir_variable *var;<br>
+ bool is_vert_array = false, is_2D_array = false;<br>
+<br>
+ /* Check whether this dereference is of a GS input array. These are special<br>
+ * because the array index refers to the index of an input vertex instead of<br>
+ * the attribute index. The exceptions to this exception are 2D arrays<br>
+ * such as gl_TexCoordIn. For these, there is a nested dereference_array,<br>
+ * where the inner index specifies the vertex and the outer index specifies<br>
+ * the attribute. To complicate things further, matrix columns are also<br>
+ * accessed with dereference_array. So we have to correctly handle 1D<br>
+ * arrays of non-matrices, 1D arrays of matrices, 2D arrays of non-matrices,<br>
+ * and 2D arrays of matrices.<br>
+ */<br>
+ if (this->shader_type == GL_GEOMETRY_SHADER) {<br>
+ if (!deref_var) {<br>
+ /* Either an outer (attribute) dereference of a 2D array or a column<br>
+ * dereference of an array of matrices. */<br>
</blockquote>
<br></div></div>
Style-nit: */ goes on a separate line.</blockquote><div><br></div><div>Fixed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ ir_dereference_array *inner_deref = ir->array->as_dereference_<u></u>array();<br>
+ assert(inner_deref);<br>
+ deref_var = inner_deref->array->as_<u></u>dereference_variable();<br>
+ is_2D_array = true;<br>
+ }<br>
+<br>
+ if (deref_var && deref_var->var->mode == ir_var_shader_in) {<br>
+ if (ir->type->is_array())<br>
+ /* Inner (vertex) dereference of a 2D array */<br>
+ return visit_continue;<br>
+ else<br>
+ /* Dereference of a 1D (vertex) array */<br>
+ is_vert_array = true;<br>
+ }<br>
+ }<br>
</blockquote>
<br></div>
I'm not terribly comfortable with this code, but I'm not sure what to suggest instead. One concern I have is varying structs.<br>
<br>
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 ...) ...)?<br>
<br>
The code above seems to assume that if it's not<br>
(array_ref (var_ref ...))<br>
then it *must* be<br>
(array_ref (array_ref ...) ...)<br>
which seems wrong, or at least dubious.<br>
<br>
The "this must be a matrix" stuff concerns me too.</blockquote><div><br></div><div>Good point. I'll look at this in more detail too.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
@@ -174,6 +177,77 @@ private:<br>
};<br>
<br>
<br>
+class geom_array_resize_visitor : public ir_hierarchical_visitor {<br>
</blockquote>
<br></div></div>
This looks like it might be the code to cross-validate<br>
<br>
layout(triangles)<br>
in vec4 foo[3];<br>
in vec4 foo[2];<br>
<br>
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.</blockquote><div><br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
@@ -1648,10 +1750,13 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)<br>
unsigned num_vert_shaders = 0;<br>
struct gl_shader **frag_shader_list;<br>
unsigned num_frag_shaders = 0;<br>
+ struct gl_shader **geom_shader_list;<br>
+ unsigned num_geom_shaders = 0;<br>
<br>
vert_shader_list = (struct gl_shader **)<br>
- calloc(2 * prog->NumShaders, sizeof(struct gl_shader *));<br>
+ calloc(3 * prog->NumShaders, sizeof(struct gl_shader *));<br>
frag_shader_list = &vert_shader_list[prog-><u></u>NumShaders];<br>
+ geom_shader_list = &vert_shader_list[prog-><u></u>NumShaders * 2];<br>
</blockquote>
<br></div></div>
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.<br>
<br>
I would much prefer to see:<br>
<br>
struct gl_shader **vert_shader_list =<br>
calloc(prog->NumShaders, sizeof(struct gl_shader *));<br>
struct gl_shader **frag_shader_list =<br>
calloc(prog->NumShaders, sizeof(struct gl_shader *));<br>
struct gl_shader **geom_shader_list =<br>
calloc(prog->NumShaders, sizeof(struct gl_shader *));<br>
<br>
and then later:<br>
<br>
free(vert_shader_list);<br>
free(frag_shader_list);<br>
free(geom_shader_list);<br>
<br>
Linking is complicated enough without accidental complexity.</blockquote><div><br></div><div>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. <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
@@ -1860,7 +1985,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)<br>
* non-zero, but the program object has no vertex or geometry<br>
* shader;<br>
*/<br>
- if (first >= MESA_SHADER_FRAGMENT) {<br>
+ if (first == MESA_SHADER_FRAGMENT) {<br>
</blockquote>
<br></div></div>
This hunk is unnecessary (but harmless).</blockquote><div><br></div><div>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?)<br>
</div></div></div></div>