[Mesa-dev] [PATCH 11/34] glsl: support compilation of geometry shaders

[Paul Berry]

On 30 July 2013 15:16, Kenneth Graunke <kenneth at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130731/227778f8/attachment.html>