[Mesa-dev] [PATCH 2/3] glsl: translate transform feedback varyings into low-level representation

[Marek Olšák]

Hi Paul,

I won't comment on the patch 1, because I didn't write it, I was just
preserving history.

On Fri, Nov 4, 2011 at 10:00 PM, Paul Berry <stereotype441 at gmail.com> wrote:
[snip]
>> +bool parse_tfeedback_decl(const void *mem_ctx, const char *input,
>> +                          struct tfeedback_decl *decl)
>> +{
>> +   /* We don't have to be pedantic about what is a valid GLSL variable
>> name,
>> +    * because any variable with an invalid name can't exist in the IR
>> anyway.
>> +    */
>> +
>> +   const char *bracket = strrchr(input, '[');
>> +
>> +   if (bracket) {
>> +      decl->name = ralloc_strndup(mem_ctx, input, bracket - input);
>> +      if (sscanf(bracket, "[%u]", &decl->array_index) == 1) {
>> +         decl->is_array = true;
>> +         return true; /* Found: "var[i]" */
>> +      }
>> +   } else {
>> +      decl->name = ralloc_strdup(mem_ctx, input);
>> +      return true;
>> +   }
>> +
>> +   return false;
>> +}
>
> For non-arrays, this function doesn't assign to decl->is_array or
> decl->array_index.  That's a problem, because its caller
> (ir_set_transform_feedback_outs::ir_set_transform_feedback_outs()) passes
> uninitialized memory for decl.  We could fix the problem by having
> ir_set_transform_feedback_outs::ir_set_transform_feedback_outs() initialize
> the memory first, but I would prefer if we fixed the problem by always
> assigning to all fields of decl, since we're effectively treating it as an
> output of this function.

Yeah, that was unintentional.

Regarding the other comments, I don't know the GLSL compiler so well.
I had forked ir_set_program_inouts and was basing upon that. Now that
I think about it, it makes more sense to do it like you say. :)

Concerning this:

varying mat4 r;
glTransformFeedbackVaryingsEXT(.. "r[1]" ..);

The EXT spec doesn't state whether this is legal. It says nothing
about matrices, actually.

[snip]
>
> The patch series doesn't add any code that calls this function.  Is that
> planned for a future patch?
>

The function is already used here:
http://cgit.freedesktop.org/~mareko/mesa/log/?h=transform-feedback

It's a work in progress, but EXT_transform_feedback already works
quite well and ARB_transform_feedback2 too except
glDrawTransformFeedback, which isn't fully implemented yet. Despite
your remarks, the GLSL linker works pretty good for me, so I am going
to take a break from it for a while.

Feel free to take over all 3 patches and implement it properly, as I
might not have time to get to it again soon.

Marek