[Mesa-dev] [PATCH] mesa: Fix transform feedback of unsubscripted arrays.

[Paul Berry]

On 4 January 2012 11:38, Ian Romanick <idr at freedesktop.org> wrote:

> On 01/03/2012 10:35 PM, Paul Berry wrote:
>
>> It is not explicitly stated in the GL 3.0 spec that transform feedback
>> can be performed on a whole varying array (without supplying a
>> subscript).  However, it seems clear from context that this was the
>> intent.  Section 2.15 (TransformFeedback) says this:
>>
>>     When writing varying variables that are arrays, individual array
>>     elements are written in order.
>>
>> And section 2.20.3 (Shader Variables), says this, in the description
>> of GetTransformFeedbackVarying:
>>
>>     For the selected varying variable, its type is returned into
>>     type. The size of the varying is returned into size. The value in
>>     size is in units of the type returned in type.
>>
>> If it were not possible to perform transform feedback on an
>> unsubscripted array, the returned size would always be 1.
>>
>> This patch fixes the linker so that transform feedback on an
>> unsubscripted array is supported.
>>
>> Fixes piglit tests "EXT_transform_feedback/**builtin-varyings
>> gl_ClipDistance[{4,8}]-no-**subscript" and
>> "EXT_transform_feedback/**output_type *[2]-no-subscript".
>>
>> Note: on back-ends that set
>> gl_shader_compiler_options::**LowerClipDistance (for example i965),
>> tests "EXT_transform_feedback/**builtin-varyings
>> gl_ClipDistance[{1,2,3,5,6,7}]**" still fail.  I hope to address this in
>> a later patch.
>>
>
> Other than the one comment below, which can be addressed later, this patch
> is
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>
>  ---
>>  src/glsl/linker.cpp |   99 ++++++++++++++++++++++++++++--**
>> ---------------------
>>  1 files changed, 54 insertions(+), 45 deletions(-)
>>
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 45edafb..883a635 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -1426,12 +1426,12 @@ private:
>>     /**
>>      * True if the declaration in orig_name represents an array.
>>      */
>> -   bool is_array;
>> +   bool is_subscripted;
>>
>>     /**
>> -    * If is_array is true, the array index that was specified in
>> orig_name.
>> +    * If is_subscripted is true, the subscript that was specified in
>> orig_name.
>>      */
>> -   unsigned array_index;
>> +   unsigned array_subscript;
>>
>>     /**
>>      * Which component to extract from the vertex shader output location
>> that
>> @@ -1460,6 +1460,12 @@ private:
>>
>>     /** Type of the varying returned by glGetTransformFeedbackVarying(**)
>> */
>>     GLenum type;
>> +
>> +   /**
>> +    * If location != -1, the size that should be returned by
>> +    * glGetTransformFeedbackVarying(**).
>> +    */
>> +   unsigned size;
>>  };
>>
>>
>> @@ -1484,14 +1490,14 @@ tfeedback_decl::init(struct gl_context *ctx,
>> struct gl_shader_program *prog,
>>
>>     if (bracket) {
>>        this->var_name = ralloc_strndup(mem_ctx, input, bracket - input);
>> -      if (sscanf(bracket, "[%u]",&this->array_index) != 1) {
>> +      if (sscanf(bracket, "[%u]",&this->array_subscript) != 1) {
>>
>>           linker_error(prog, "Cannot parse transform feedback varying
>> %s", input);
>>           return false;
>>        }
>> -      this->is_array = true;
>> +      this->is_subscripted = true;
>>     } else {
>>        this->var_name = ralloc_strdup(mem_ctx, input);
>> -      this->is_array = false;
>> +      this->is_subscripted = false;
>>     }
>>
>>     /* For drivers that lower gl_ClipDistance to gl_ClipDistanceMESA, we
>> need
>> @@ -1501,8 +1507,10 @@ tfeedback_decl::init(struct gl_context *ctx,
>> struct gl_shader_program *prog,
>>     if (ctx->ShaderCompilerOptions[**MESA_SHADER_VERTEX].**
>> LowerClipDistance&&
>>         strcmp(this->var_name, "gl_ClipDistance") == 0) {
>>        this->var_name = "gl_ClipDistanceMESA";
>> -      this->single_component = this->array_index % 4;
>> -      this->array_index /= 4;
>> +      if (this->is_subscripted) {
>> +         this->single_component = this->array_subscript % 4;
>> +         this->array_subscript /= 4;
>> +      }
>>     }
>>
>>     return true;
>> @@ -1518,9 +1526,9 @@ tfeedback_decl::is_same(const tfeedback_decl&x,
>> const tfeedback_decl&y)
>>  {
>>     if (strcmp(x.var_name, y.var_name) != 0)
>>        return false;
>> -   if (x.is_array != y.is_array)
>> +   if (x.is_subscripted != y.is_subscripted)
>>        return false;
>> -   if (x.is_array&&  x.array_index != y.array_index)
>> +   if (x.is_subscripted&&  x.array_subscript != y.array_subscript)
>>
>>        return false;
>>     if (x.single_component != y.single_component)
>>        return false;
>> @@ -1542,37 +1550,39 @@ tfeedback_decl::assign_**location(struct
>> gl_context *ctx,
>>  {
>>     if (output_var->type->is_array()) {
>>        /* Array variable */
>> -      if (!this->is_array) {
>> -         linker_error(prog, "Transform feedback varying %s found, "
>> -                      "but array dereference required for varying
>> %s[%d]).",
>> -                      this->orig_name,
>> -                     output_var->name, output_var->type->length);
>> -         return false;
>> -      }
>> -      /* Check array bounds. */
>> -      if (this->array_index>=
>> -          (unsigned) output_var->type->array_size()**) {
>> -         linker_error(prog, "Transform feedback varying %s has index "
>> -                      "%i, but the array size is %i.",
>> -                      this->orig_name, this->array_index,
>> -                      output_var->type->array_size()**);
>> -         return false;
>> -      }
>>        const unsigned matrix_cols =
>>           output_var->type->fields.**array->matrix_columns;
>> -      this->location = output_var->location + this->array_index *
>> matrix_cols;
>> +
>> +      if (this->is_subscripted) {
>> +         /* Check array bounds. */
>> +         if (this->array_subscript>=
>> +             (unsigned) output_var->type->array_size()**) {
>> +            linker_error(prog, "Transform feedback varying %s has index "
>> +                         "%i, but the array size is %i.",
>> +                         this->orig_name, this->array_subscript,
>> +                         output_var->type->array_size()**);
>> +            return false;
>> +         }
>> +         this->location =
>> +            output_var->location + this->array_subscript * matrix_cols;
>> +         this->size = 1;
>> +      } else {
>> +         this->location = output_var->location;
>> +         this->size = (unsigned) output_var->type->array_size()**;
>> +      }
>>        this->vector_elements = output_var->type->fields.**
>> array->vector_elements;
>>        this->matrix_columns = matrix_cols;
>> -      this->type = GL_NONE;
>> +      this->type = output_var->type->fields.**array->gl_type;
>>     } else {
>>        /* Regular variable (scalar, vector, or matrix) */
>> -      if (this->is_array) {
>> +      if (this->is_subscripted) {
>>           linker_error(prog, "Transform feedback varying %s found, "
>>                        "but it's an array ([] expected).",
>>                        this->orig_name);
>>
>
> I hadn't noticed this before, but this error message seems to not match
> the condition.  The output variable (the varying) is not an array, but the
> transform feedback setting included an array subscript.  Right?
>
>
You're right, that's a bogus error message.  I'll submit a follow-up patch
that fixes it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120104/2a91f614/attachment.html>