<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">
@@ -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></div></div><div><div>
<br></div></div>
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.<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">
+ if (this->shader_type == GL_GEOMETRY_SHADER &&<br>
+ ir->var->mode == ir_var_shader_in) {<br>
+ if (ir->type->element_type()->is_<u></u>array()) {<br>
+ const glsl_type *inner_array_type = ir->type->fields.array;<br>
+ matrix_columns = inner_array_type->fields.<u></u>array->matrix_columns;<br>
+ length = inner_array_type->length;<br>
+ } else {<br>
+ length = 1;<br>
+ }<br>
+ }<br>
+ mark(this->prog, ir->var, 0, length * matrix_columns,<br>
+ this->shader_type == GL_FRAGMENT_SHADER);<br>
} else {<br>
mark(this->prog, ir->var, 0, ir->type->matrix_columns,<br>
- this->is_fragment_shader);<br>
+ this->shader_type == GL_FRAGMENT_SHADER);<br>
}<br>
-<br>
return visit_continue;<br>
}<br>
<br>
@@ -129,7 +138,40 @@ ir_set_program_inouts_visitor:<u></u>:visit_enter(ir_dereference_<u></u>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.<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>I believe that we don't need to worry about structs, because lower_packed_varyings() gets rid of them before this analysis runs. But that assumption isn't documented (or checked) anywhere, and it really should be.<br>
<br></div><div>Even with that, I had a very difficult time figuring out whether these modifications to ir_set_program_inouts were correct, so I decided to rework it. It was easy to isolate this from the rest of the patch series, so I'm going to send it to the list as a series of its own. Hopefully the rework will be easier to convince ourselves that the rework is correct.<br>
</div></div></div></div>