On 30 July 2013 15:16, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On 07/28/2013 11:03 PM, Paul Berry wrote:
>
>> @@ -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.
>
>
> + if (this->shader_type == GL_GEOMETRY_SHADER &&
>> + ir->var->mode == ir_var_shader_in) {
>> + if (ir->type->element_type()->is_**array()) {
>> + const glsl_type *inner_array_type = ir->type->fields.array;
>> + matrix_columns = inner_array_type->fields.**
>> array->matrix_columns;
>> + length = inner_array_type->length;
>> + } else {
>> + length = 1;
>> + }
>> + }
>> + mark(this->prog, ir->var, 0, length * matrix_columns,
>> + this->shader_type == GL_FRAGMENT_SHADER);
>> } else {
>> mark(this->prog, ir->var, 0, ir->type->matrix_columns,
>> - this->is_fragment_shader);
>> + this->shader_type == GL_FRAGMENT_SHADER);
>> }
>> -
>> return visit_continue;
>> }
>>
>> @@ -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.
>
>
> + 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.
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.
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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
