On 4 January 2012 11:38, Ian Romanick <i...@freedesktop.org> wrote:
> On 01/03/2012 10:35 PM, Paul Berry wrote:
>
>> 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.
>>
>
> Other than the one comment below, which can be addressed later, this patch
> is
>
> Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
>
> ---
>> 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);
>> - if (sscanf(bracket, "[%u]",&this->array_index) != 1) {
>> + if (sscanf(bracket, "[%u]",&this->array_subscript) != 1) {
>>
>> 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;
>> - if (x.is_array&& x.array_index != y.array_index)
>> + if (x.is_subscripted&& x.array_subscript != y.array_subscript)
>>
>> 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);
>>
>
> 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?
>
>
You're right, that's a bogus error message. I'll submit a follow-up patch
that fixes it.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
