<div class="gmail_quote">On 8 November 2011 05:15, Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> <div class="HOEnZb"><div class="h5">On Tue, Nov 8, 2011 at 7:28 AM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: > This patch modifies the GLSL linker to assign additional slots for > varying variables used by transform feedback, and record the varying > slots used by transform feedback for use by the driver back-end. > > This required modifying assign_varying_locations() so that it assigns > a varying location if either (a) the varying is used by the next stage > of the GL pipeline, or (b) the varying is required by transform > feedback. In order to avoid duplicating the code to assign a single > varying location, I moved it into its own function, > assign_varying_location(). > > In addition, to support transform feedback in the case where there is > no fragment shader, it is now possible to call > assign_varying_locations() with a consumer of NULL. > --- > Changes from v1: > > - Fixed loop bound in tfeedback_decl::store() (was this->vector_elements, > should have been this->matrix_columns). > > - Fixed the case where transform feedback is in use but there is no fragment > shader. > > src/glsl/linker.cpp | 552 ++++++++++++++++++++++++++++++++++++++++++------ > src/mesa/main/mtypes.h | 13 ++ > 2 files changed, 502 insertions(+), 63 deletions(-) > > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp > index 915d5bb..5cccd7f 100644 > --- a/src/glsl/linker.cpp > +++ b/src/glsl/linker.cpp > @@ -1519,10 +1519,358 @@ demote_shader_inputs_and_outputs(gl_shader *sh, enum ir_variable_mode mode) > } > > > +/** > + * Data structure tracking information about a transform feedback declaration > + * during linking. > + */ > +class tfeedback_decl > +{ > +public: > + bool init(struct gl_shader_program *prog, const void *mem_ctx, > + const char *input); > + static bool is_same(const tfeedback_decl &x, const tfeedback_decl &y); > + bool assign_location(struct gl_context *ctx, struct gl_shader_program *prog, > + ir_variable *output_var); > + bool store(struct gl_shader_program *prog, > + struct gl_transform_feedback_info *info, unsigned buffer) const; > + > + > + /** > + * True if assign_location() has been called for this object. > + */ > + bool is_assigned() const > + { > + return this->location != -1; > + } > + > + /** > + * Determine whether this object refers to the variable var. > + */ > + bool matches_var(ir_variable *var) const > + { > + return strcmp(var->name, this->var_name) == 0; > + } > + > + /** > + * The total number of varying components taken up by this variable. Only > + * valid if is_assigned() is true. > + */ > + unsigned num_components() const > + { > + return this->vector_elements * this->matrix_columns; > + } > + > +private: > + /** > + * The name that was supplied to glTransformFeedbackVaryings. Used for > + * error reporting. > + */ > + const char *orig_name; > + > + /** > + * The name of the variable, parsed from orig_name. > + */ > + char *var_name; > + > + /** > + * True if the declaration in orig_name represents an array. > + */ > + bool is_array; > + > + /** > + * If is_array is true, the array index that was specified in orig_name. > + */ > + unsigned array_index; > + > + /** > + * The vertex shader output location that the linker assigned for this > + * variable. -1 if a location hasn't been assigned yet. > + */ > + int location; > + > + /** > + * If location != -1, the number of vector elements in this variable, or 1 > + * if this variable is a scalar. > + */ > + unsigned vector_elements; > + > + /** > + * If location != -1, the number of matrix columns in this variable, or 1 > + * if this variable is not a matrix. > + */ > + unsigned matrix_columns; > +}; > + > + > +/** > + * Initialize this object based on a string that was passed to > + * glTransformFeedbackVaryings. If there is a parse error, the error is > + * reported using linker_error(), and false is returned. > + */ > +bool > +tfeedback_decl::init(struct gl_shader_program *prog, const void *mem_ctx, > + const char *input) > +{ > + /* 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. > + */ > + > + this->location = -1; > + this->orig_name = input; > + > + const char *bracket = strrchr(input, '['); > + > + if (bracket) { > + this->var_name = ralloc_strndup(mem_ctx, input, bracket - input); > + if (sscanf(bracket, "[%u]", &this->array_index) == 1) { > + this->is_array = true; > + return true; > + } > + } else { > + this->var_name = ralloc_strdup(mem_ctx, input); > + this->is_array = false; > + return true; > + } > + > + linker_error(prog, "Cannot parse transform feedback varying %s", input); > + return false; > +} > + > + > +/** > + * Determine whether two tfeedback_decl objects refer to the same variable and > + * array index (if applicable). > + */ > +bool > +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) > + return false; > + if (x.is_array && x.array_index != y.array_index) > + return false; > + return true; > +} > + > + > +/** > + * Assign a location for this tfeedback_decl object based on the location > + * assignment in output_var. > + * > + * If an error occurs, the error is reported through linker_error() and false > + * is returned. > + */ > +bool > +tfeedback_decl::assign_location(struct gl_context *ctx, > + struct gl_shader_program *prog, > + ir_variable *output_var) > +{ > + if (output_var->type->is_array()) { > + /* Array variable */ > + if (!this->is_array) { > + linker_error(prog, "Transform feedback varying %s found, " > + "but it's not an array ([] not expected).", > + this->orig_name); > + 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()); > + } > + const unsigned matrix_cols = > + output_var->type->fields.array->matrix_columns; > + this->location = output_var->location + this->array_index * matrix_cols; > + this->vector_elements = output_var->type->fields.array->vector_elements; > + this->matrix_columns = matrix_cols; > + } else { > + /* Regular variable (scalar, vector, or matrix) */ > + if (this->is_array) { > + linker_error(prog, "Transform feedback varying %s found, " > + "but it's an array ([] expected).", > + this->orig_name); > + return false; > + } > + this->location = output_var->location; > + this->vector_elements = output_var->type->vector_elements; > + this->matrix_columns = output_var->type->matrix_columns; > + } > + /* From GL_EXT_transform_feedback: > + * A program will fail to link if: > + * > + * * the total number of components to capture in any varying > + * variable in <varyings> is greater than the constant > + * MAX_TRANSFORM_FEEDBACK_SEPARATE_COMPONENTS_EXT and the > + * buffer mode is SEPARATE_ATTRIBS_EXT; > + */ > + if (prog->TransformFeedback.BufferMode == GL_SEPARATE_ATTRIBS && > + this->num_components() > > + ctx->Const.MaxTransformFeedbackSeparateComponents) { > + linker_error(prog, "Transform feedback varying %s exceeds " > + "MAX_TRANSFORM_FEEDBACK_SEPARATE_COMPONENTS.", > + this->orig_name); > + return false; > + } > + > + return true; > +} > + > + > +/** > + * Update gl_transform_feedback_info to reflect this tfeedback_decl. > + * > + * If an error occurs, the error is reported through linker_error() and false > + * is returned. > + */ > +bool > +tfeedback_decl::store(struct gl_shader_program *prog, > + struct gl_transform_feedback_info *info, > + unsigned buffer) const > +{ > + if (!this->is_assigned()) { > + /* From GL_EXT_transform_feedback: > + * A program will fail to link if: > + * > + * * any variable name specified in the <varyings> array is not > + * declared as an output in the geometry shader (if present) or > + * the vertex shader (if no geometry shader is present); > + */ > + linker_error(prog, "Transform feedback varying %s undeclared.", > + this->orig_name); > + return false; > + } > + for (unsigned v = 0; v < this->matrix_columns; ++v) { > + info->Outputs[info->NumOutputs].OutputRegister = this->location + v; > + info->Outputs[info->NumOutputs].NumComponents = this->vector_elements; > + info->Outputs[info->NumOutputs].OutputBuffer = buffer; > + ++info->NumOutputs; > + } > + return true; > +} > + > + > +/** > + * Parse all the transform feedback declarations that were passed to > + * glTransformFeedbackVaryings() and store them in tfeedback_decl objects. > + * > + * If an error occurs, the error is reported through linker_error() and false > + * is returned. > + */ > +static bool > +parse_tfeedback_decls(struct gl_shader_program *prog, const void *mem_ctx, > + unsigned num_names, char **varying_names, > + tfeedback_decl *decls) > +{ > + for (unsigned i = 0; i < num_names; ++i) { > + if (!decls[i].init(prog, mem_ctx, varying_names[i])) > + return false; > + /* From GL_EXT_transform_feedback: > + * A program will fail to link if: > + * > + * * any two entries in the <varyings> array specify the same varying > + * variable; > + * > + * We interpret this to mean "any two entries in the <varyings> array > + * specify the same varying variable and array index", since transform > + * feedback of arrays would be useless otherwise. > + */ > + for (unsigned j = 0; j < i; ++j) { > + if (tfeedback_decl::is_same(decls[i], decls[j])) { > + linker_error(prog, "Transform feedback varying %s specified " > + "more than once.", varying_names[i]); > + } > + return false; </div></div>The "return false" statement should be put next to linker_error. Otherwise it always fails when the number of TFB varyings is at least 2. </blockquote><div> </div><div>Whoops, you're right of course.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> With that fixed, this patch is: Reviewed-by: Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> Tested-by: Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> </blockquote><div> </div><div>Thanks for the quick turnaround, Marek. I'll wait until the end of the day to push this upstream, in case anyone else has any review comments.</div> <div> </div><div>Paul</div></div>