[Mesa-dev] [PATCH] glsl: Remove field array_lvalue from ir_variable.

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/12/2011 06:20 PM, Paul Berry wrote:
> The array_lvalue field was attempting to enforce the restriction
> that whole arrays can't be used on the left-hand side of an
> assignment in GLSL 1.10 or GLSL ES, and can't be used as out or
> inout parameters in GLSL 1.10.
> 
> However, it was buggy (it didn't work properly for built-in
> arrays), and it was clumsy (it unnecessarily kept track on a 
> variable-by-variable basis, and it didn't cover the GLSL ES case).
> 
> This patch removes the array_lvalue field completely in favor of 
> explicit checks in ast_parameter_declarator::hir() (this check is 
> added) and in do_assignment (this check was already present).
> 
> This causes a benign behavioral change: when the user attempts to
> pass an array as an out or inout parameter of a function in GLSL
> 1.10, the error is now flagged at the time the function definition
> is encountered, rather than at the time of invocation.  Previously
> we allowed such functions to be defined, and only flagged the error
> if they were invoked.

Other than the one comment below,

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> Fixes Piglit tests 
> spec/glsl-1.10/compiler/qualifiers/fn-{out,inout}-array-prohibited*
>
> 
and
> spec/glsl-1.20/compiler/assignment-operators/assign-builtin-array-allowed.vert.
>
> 
- ---
> src/glsl/ast_to_hir.cpp |   39
> ++++++++++++++++++++------------------- src/glsl/ir.cpp         |
> 5 +---- src/glsl/ir.h           |    8 -------- 
> src/glsl/ir_clone.cpp   |    1 - 4 files changed, 21 insertions(+),
> 32 deletions(-)
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp 
> index 484786c..579426b 100644 --- a/src/glsl/ast_to_hir.cpp +++
> b/src/glsl/ast_to_hir.cpp @@ -2120,25 +2120,6 @@
> apply_type_qualifier_to_variable(const struct ast_type_qualifier
> *qual, var->depth_layout = ir_depth_layout_unchanged; else 
> var->depth_layout = ir_depth_layout_none; - -   /* From page 46
> (page 52 of the PDF) of the GLSL ES specification: -    * -    *
> "Array variables are l-values and may be passed to parameters -
> *     declared as out or inout. However, they may not be used as -
> *     the target of an assignment." -    * -    * From page 32
> (page 38 of the PDF) of the GLSL 1.10 spec: -    * -    *    "Other
> binary or unary expressions, non-dereferenced arrays, -    *
> function names, swizzles with repeated fields, and constants -    *
> cannot be l-values." -    * -    * So we only mark 1.10 as
> non-lvalues, and check for array -    * assignment in 100
> specifically in do_assignment. -    */ -   if
> (var->type->is_array() && state->language_version != 110) { -
> var->array_lvalue = true; -   } }
> 
> /** @@ -2953,6 +2934,26 @@ ast_parameter_declarator::hir(exec_list
> *instructions, type = glsl_type::error_type; }
> 
> +   /* From page 39 (page 45 of the PDF) of the GLSL 1.10 spec: +
> * +    *    "When calling a function, expressions that do not
> evaluate to +    *     l-values cannot be passed to parameters
> declared as out or inout." +    * +    * From page 32 (page 38 of
> the PDF) of the GLSL 1.10 spec: +    * +    *    "Other binary or
> unary expressions, non-dereferenced arrays, +    *     function
> names, swizzles with repeated fields, and constants +    *
> cannot be l-values." +    * +    * So for GLSL 1.10, passing an
> array as an out or inout parameter is not +    * allowed.  This
> restriction is removed in GLSL 1.20, and in GLSL ES. +    */ +   if
> ((var->mode == ir_var_inout || var->mode == ir_var_out) +       &&
> type->is_array() && state->language_version == 110) { +
> _mesa_glsl_error(&loc, state, "Arrays are not l-values in GLSL
> 1.10");

The error message should say what the actual error is.  While this
message is technically correct, it will confuse / irritate some
people.  "Arrays cannot be out or inout parameters in GLSL 1.10" is
better.

> +      type = glsl_type::error_type; +   } + 
> instructions->push_tail(var);
> 
> /* Parameter declarations do not have r-values. diff --git
> a/src/glsl/ir.cpp b/src/glsl/ir.cpp index 41ed4f1..d6594cd 100644 
> --- a/src/glsl/ir.cpp +++ b/src/glsl/ir.cpp @@ -1105,9 +1105,6 @@
> ir_dereference::is_lvalue() const if ((var == NULL) ||
> var->read_only) return false;
> 
> -   if (this->type->is_array() && !var->array_lvalue) -      return
> false; - /* From page 17 (page 23 of the PDF) of the GLSL 1.20
> spec: * *    "Samplers cannot be treated as l-values; hence cannot
> be used @@ -1323,7 +1320,7 @@ ir_swizzle::variable_referenced()
> const ir_variable::ir_variable(const struct glsl_type *type, const
> char *name, ir_variable_mode mode) : max_array_access(0),
> read_only(false), centroid(false), invariant(false), -
> mode(mode), interpolation(ir_var_smooth), array_lvalue(false) +
> mode(mode), interpolation(ir_var_smooth) { this->ir_type =
> ir_type_variable; this->type = type; diff --git a/src/glsl/ir.h
> b/src/glsl/ir.h index 2e899f3..cc4e680 100644 --- a/src/glsl/ir.h 
> +++ b/src/glsl/ir.h @@ -346,14 +346,6 @@ public: unsigned
> interpolation:2;
> 
> /** -    * Flag that the whole array is assignable -    * -    * In
> GLSL 1.20 and later whole arrays are assignable (and comparable
> for -    * equality).  This flag enables this behavior. -    */ -
> unsigned array_lvalue:1; - -   /** * \name
> ARB_fragment_coord_conventions * @{ */ diff --git
> a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp index
> f075736..c1befa9 100644 --- a/src/glsl/ir_clone.cpp +++
> b/src/glsl/ir_clone.cpp @@ -47,7 +47,6 @@ ir_variable::clone(void
> *mem_ctx, struct hash_table *ht) const var->centroid =
> this->centroid; var->invariant = this->invariant; 
> var->interpolation = this->interpolation; -   var->array_lvalue =
> this->array_lvalue; var->location = this->location; 
> var->warn_extension = this->warn_extension; var->origin_upper_left
> = this->origin_upper_left;

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5vZe8ACgkQX1gOwKyEAw/7CwCdGruEaoHHvw24Yfx+x47GXzaw
HjQAnj9PKhN5oVUFPjLk3z8kjI0cP3cB
=NAXE
-----END PGP SIGNATURE-----