[Mesa-dev] [PATCH 1/3] glsl: Assign transform feedback varying slots in linker.

[Paul Berry]

Sorry I missed the first round of feedback on these patches.  I hope my
comments aren't coming too late.

On 1 November 2011 12:48, Marek Olšák <maraeo at gmail.com> wrote:

> From: Dan McCabe <zen3d.linux at gmail.com>
>
> Modify the linker to assign additional slots for varying
> variables used by transform feedback. This is done after other
> varyings are already assigned slots.
>
> Since this is done after previous varying slot assignments,
> the code needs to know how many varyings are already assigned
> slots. A new function "max_varying()" is introduced to examine a
> previously processed shader to find the largest varying already
> assigned a slot. All new varyings will be assigned slots after
> this one. If varyings are found, -1 is returned.
>
> [ Marek Olšák: fix segfault in accessing names ]
> ---
>  src/glsl/linker.cpp |   84
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 84 insertions(+), 0 deletions(-)
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index beadec6..e6012bf 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -207,6 +207,25 @@ invalidate_variable_locations(gl_shader *sh, enum
> ir_variable_mode mode,
>    }
>  }
>
> +
> +int
> +max_varying(gl_shader *sh, enum ir_variable_mode mode)
> +{
> +   int max_varying = -1;
> +
> +   foreach_list(node, sh->ir) {
> +      ir_variable *const var = ((ir_instruction *) node)->as_variable();
> +
> +      if ((var == NULL) || (var->mode != (unsigned) mode))
> +        continue;
> +
> +      if (var->location > max_varying)
> +        max_varying = var->location;
> +   }
> +
> +   return max_varying;
> +}
>

A few comments on this function:

1. It should be declared static, since it's only used in this file.

2. Since it's only called with mode == ir_var_out, it might make more sense
to drop that argument and call the function max_out_location().

3. Judging by how the return value of this function is used (it has 1 added
to it and then the value is adjusted to be at least VERT_RESULT_VAR0), it
seems like the real purpose of this function is to figure out the first
slot into which varyings can be safely assigned.  Consider renaming this
function to something like "find_free_varying_slots" and having it return
the first slot into which varyings can be safely assigned.

4. This function might be unnecessary--see my comment below.


> +
>
>  /**
>  * Determine the number of attribute slots required for a particular type
> @@ -1597,6 +1616,69 @@ assign_varying_locations(struct gl_context *ctx,
>
>
>  void
> +assign_transform_feedback_varying_locations(struct gl_context *ctx,
> +                                           struct gl_shader_program
> *prog)
>
+{
> +   struct gl_shader *vs = prog->_LinkedShaders[MESA_SHADER_VERTEX];
> +
> +   if (vs == NULL)
> +      return;
> +
> +   char **names = prog->TransformFeedback.VaryingNames;
> +   int num_names = prog->TransformFeedback.NumVarying;
> +
> +   if (num_names <= 0)
> +      return;
> +
> +   int num_varying = max_varying(vs, ir_var_out) + 1;
> +   unsigned output_index =
> +     (num_varying > VERT_RESULT_VAR0)
> +        ? num_varying
> +        : VERT_RESULT_VAR0;
>

The effect of this code is to just recompute the value that output_index
had when assign_varying_locations() finished.  Wouldn't it be better to
just have assign_varying_locations() just store the value of output_index
somewhere so we can retrieve it here?

Or, perhaps it would be better to merge all of this code into
assign_varying_locations().  Really, both of these functions are doing the
same task, they are just using different criteria (assign_varying_locations
assigns locations to vertex shader outputs if they are used by the fragment
shader, this function assigns locations to vertex shader outputs if they
are used by transform feedback).


> +
> +   foreach_list(node, vs->ir) {
> +      ir_variable *const output_var = ((ir_instruction *)
> node)->as_variable();
> +
> +      if (output_var == NULL
> +      ||  output_var->mode != ir_var_out
> +      ||  output_var->location != -1)
> +        continue;
> +
> +      /* Find a transform feedback varying variable that has
> +       * the same name as the shader variable.
> +       */
> +      int varying_index = -1;
> +      for (int num_name = 0; num_name < num_names; num_name++) {
> +         char *name = names[num_name];
> +         if (strcmp(output_var->name, name) == 0) {
> +            varying_index = num_name;
> +            break;
> +         }
> +      }
> +
> +      if (varying_index == -1)
> +        continue;
> +
> +      output_var->location = output_index;
> +
> +      /* FINISHME: Support for "varying" records in GLSL 1.50. */
> +      assert(!output_var->type->is_record());
> +
> +      if (output_var->type->is_array()) {
> +        const unsigned slots = output_var->type->length
> +           * output_var->type->fields.array->matrix_columns;
> +
> +        output_index += slots;
> +      } else {
> +        const unsigned slots = output_var->type->matrix_columns;
> +
> +        output_index += slots;
> +      }
>

Aside: I have just realized that the technique we used to implement the
gl_ClipDistance[] array (to lower it from an array of 8 floats to an array
of 2 vec4s) is going to throw a monkeywrench into transform feedback of
gl_ClipDistance.  I'll make sure to add that to the transform feedback
tests I'm writing.


> +   }
> +}
>

I don't see any error checking to make sure that we don't exceed the value
of ctx->Const.MaxVarying.  (According to the EXT_transform_feedback spec,
"Varying variables specified by TransformFeedbackVaryingsEXT are always
considered active and count against the total limit on the number of active
varying components, regardless of the needs of the fragment shader.").
Normally this check would be done in assign_varying_locations(), but that
function finishes before this one is called.  This is another argument,
IMHO, for merging the two functions together.


> +
> +
> +void
>  link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>  {
>    void *mem_ctx = ralloc_context(NULL); // temporary linker context
> @@ -1778,6 +1860,8 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
>       prev = i;
>    }
>
> +   assign_transform_feedback_varying_locations(ctx, prog);
> +
>    if (prog->_LinkedShaders[MESA_SHADER_VERTEX] != NULL) {
>
> demote_shader_inputs_and_outputs(prog->_LinkedShaders[MESA_SHADER_VERTEX],
>                                       ir_var_out);
> --
> 1.7.4.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111104/d6eb8545/attachment-0001.html>