On 4 January 2012 11:38, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">On 01/03/2012 10:35 PM, Paul Berry wrote: </div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote> </div> Other than the one comment below, which can be addressed later, this patch is Reviewed-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> --- 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); </div></div> - if (sscanf(bracket, "[%u]",&this->array_index) != 1) { + if (sscanf(bracket, "[%u]",&this->array_subscript) != 1) {<div><div class="h5"> 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; </div></div> - if (x.is_array&& x.array_index != y.array_index) + if (x.is_subscripted&& x.array_subscript != y.array_subscript)<div><div class="h5"> 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); </div></div></blockquote> 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?<div class="HOEnZb"> <div class="h5"> </div></div></blockquote><div> You're right, that's a bogus error message. I'll submit a follow-up patch that fixes it. </div></div>