[Mesa-dev] [PATCH 8/8] glsl: Modify ir_set_program_inouts to handle geometry shaders.

[Paul Berry]

On 1 August 2013 16:03, Ian Romanick <idr at freedesktop.org> wrote:

> A couple of comments below.  Other than that,
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>
>
> On 07/31/2013 02:18 PM, Paul Berry wrote:
>
>> ---
>>   src/glsl/ir_set_program_**inouts.cpp | 85
>> ++++++++++++++++++++++++++++++**++------
>>   1 file changed, 73 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/glsl/ir_set_program_**inouts.cpp
>> b/src/glsl/ir_set_program_**inouts.cpp
>> index 018342b..8f96324 100644
>> --- a/src/glsl/ir_set_program_**inouts.cpp
>> +++ b/src/glsl/ir_set_program_**inouts.cpp
>> @@ -114,7 +114,13 @@ mark(struct gl_program *prog, ir_variable *var, int
>> offset, int len,
>>   void
>>   ir_set_program_inouts_visitor:**:mark_whole_variable(ir_**variable
>> *var)
>>   {
>> -   mark(this->prog, var, 0, var->type->count_attribute_**slots(),
>> +   const glsl_type *type = var->type;
>> +   if (this->shader_type == GL_GEOMETRY_SHADER &&
>> +       var->mode == ir_var_shader_in && type->is_array()) {
>> +      type = type->fields.array;
>> +   }
>> +
>> +   mark(this->prog, var, 0, type->count_attribute_slots(),
>>           this->shader_type == GL_FRAGMENT_SHADER);
>>   }
>>
>> @@ -133,7 +139,9 @@ ir_set_program_inouts_visitor:**:visit(ir_dereference_variable
>> *ir)
>>   /**
>>    * Try to mark a portion of the given variable as used.  Caller must
>> ensure
>>    * that the variable represents a shader input or output which can be
>> indexed
>> - * into in array fashion (an array, matrix, or vector).
>> + * into in array fashion (an array, matrix, or vector).  For the purpose
>> of
>> + * geometry shader inputs (which are always arrays), this means that the
>> array
>> + * element must be something that can be indexed into in array fashion.
>>
>
> Except gl_PrimitiveIDIn, as noted below.


Good point.  I've updated the comment.


>
>
>     *
>>    * If the index can't be interpreted as a constant, or some other
>> problem
>>    * occurs, then nothing will be marked and false will be returned.
>> @@ -144,6 +152,16 @@ ir_set_program_inouts_visitor:**
>> :try_mark_partial_variable(ir_**variable *var,
>>   {
>>      const glsl_type *type = var->type;
>>
>> +   if (this->shader_type == GL_GEOMETRY_SHADER &&
>> +       var->mode == ir_var_shader_in) {
>> +      /* The only geometry shader input that is not an array is
>> +       * gl_PrimitiveIDIn, and in that case, this code will never be
>> reached,
>> +       * because gl_PrimitiveIDIn can't be indexed into in array fashion.
>> +       */
>> +      assert(type->is_array());
>> +      type = type->fields.array;
>> +   }
>> +
>>      /* The code below only handles:
>>       *
>>       * - Indexing into matrices
>> @@ -205,17 +223,60 @@ ir_set_program_inouts_visitor:**
>> :try_mark_partial_variable(ir_**variable *var,
>>   ir_visitor_status
>>   ir_set_program_inouts_visitor:**:visit_enter(ir_dereference_**array
>> *ir)
>>   {
>> -   ir_dereference_variable *deref_var;
>> -   deref_var = ir->array->as_dereference_**variable();
>> -   ir_variable *var = deref_var ? deref_var->var : NULL;
>> -
>> -   /* Check that we're dereferencing a shader in or out */
>> -   if (!var || !is_shader_inout(var))
>> -      return visit_continue;
>> -
>> -   if (try_mark_partial_variable(**var, ir->array_index))
>> -      return visit_continue_with_parent;
>> +   /* Note: for geometry shader inputs, lower_named_interface_blocks may
>> +    * create 2D arrays, so we need to be able to handle those.  2D arrays
>> +    * shouldn't be able to crop up for any other reason.
>> +    */
>> +   if (ir_dereference_array *inner_array =
>> +       ir->array->as_dereference_**array()) {
>>
>
> 'ir_dereference_array *const'?
>
> I'm not sure whether I love or hate this style.  I seem some significant
> advantages to it (the big one being the limited scope of the variable), but
> it has some stylistic issues (the long if-condition).  Having an assignment
> in an if-condition also feels icky.
>
> I guess it's pretty similar to what we already do with for-loops, so I'm
> leaning towards liking it.


Those are all fair points.  I'm leaning towards liking it too, so I'm going
to go ahead with it.  If there are strong objections later, we can always
change it.

I like the idea of marking these declarations-inside-if-statements as
const--it reduces the potential ick a little bit by clarifying that we're
not going to do anything dangerous with them :)

Thanks again for all the code review on these two huge patch series.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130801/61f0d951/attachment-0001.html>