[Mesa-dev] [PATCH] glsl: Remove field array_lvalue from ir_variable.
Marek Olšák
maraeo at gmail.com
Fri Sep 16 09:01:22 PDT 2011
Hi Paul,
this commit breaks the piglit test
shaders/glsl-arb-fragment-coord-conventions on softpipe and r600g:
bin/glsl-arb-fragment-coord-conventions -auto
Regular gl_FragCoord
Pixel center half integer
Pixel center integer
Probe at (0,0)
Expected: 0.250000 0.250000 0.000000
Observed: 0.749020 0.749020 0.000000
Probe at (99,99)
Expected: 0.250000 0.250000 0.000000
Observed: 0.749020 0.749020 0.000000
Pixel origin upper left
Probe at (0,0)
Expected: 0.000000 1.000000 0.000000
Observed: 0.003922 0.003922 0.000000
Probe at (99,99)
Expected: 1.000000 0.000000 0.000000
Observed: 0.996078 0.996078 0.000000
Pixel origin upper left and pixel center integer
PIGLIT: {'result': 'fail' }
Reverting the commit fixes this. It looks like
layout(pixel_center_integer) is ignored, because "Pixel center half
integer" and "Pixel center integer" return the same color in the test.
Do you have any idea what's wrong?
Marek
On Tue, Sep 13, 2011 at 1:20 AM, Paul Berry <stereotype441 at gmail.com> 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.
>
> 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");
> + 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;
> --
> 1.7.6
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list