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

[Paul Berry]

On 4 November 2011 16:43, Marek Olšák <maraeo at gmail.com> wrote:

> 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.
>

Ok, I've worked up an alternative proposal, which I'll send out shortly.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111107/dfd0e033/attachment.htm>