On 17 July 2013 18:24, Kenneth Graunke <kenn...@whitecape.org> wrote:
> Rather than creating a new "binding" field in ir_variable, we reuse
> constant_value since the linker code for handling uniform initializers
> uses that.
>
I'm uncomfortable with this. It's already a bit irregular that we use
constant_value for this purpose in handling uniform initializers (since the
value of a uniform initializer can be overridden using the GL API, and
hence isn't really constant), and we already have some ugly workaround code
to deal with that irregularity (see
ir_dereference_variable::constant_expression_value(), which has to have a
special case to ignore constant_value when looking at a uniform). In point
of fact, I'm worried that there may be bugs in opt_constant_variable.cpp,
because it doesn't seem to go to any effort to avoid trying to treat
uniforms with initializers as constants.
Re-using constant_value for bindings seems even more dangerous, since now
the types won't even match.
>
> Since UBOs and samplers can't otherwise have initializers/constant
> values, there shouldn't be a conflict.
>
> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org>
> ---
> src/glsl/ast_to_hir.cpp | 7 +++++--
> src/glsl/ir.h | 8 ++++++++
> src/glsl/ir_clone.cpp | 1 +
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 4a04b60..f438b6d 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2158,8 +2158,11 @@ apply_type_qualifier_to_variable(const struct
> ast_type_qualifier *qual,
> "explicit index requires explicit location\n");
> }
>
> - if (qual->flags.q.explicit_binding)
> - validate_binding_qualifier(state, loc, var, qual);
> + if (qual->flags.q.explicit_binding &&
> + validate_binding_qualifier(state, loc, var, qual)) {
> + var->explicit_binding = true;
> + var->constant_value = new(state) ir_constant(qual->binding);
> + }
>
> /* Does the declaration use the deprecated 'attribute' or 'varying'
> * keywords?
> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> index 1f0dc09..46bdb38 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -471,6 +471,14 @@ public:
> unsigned explicit_index:1;
>
> /**
> + * Was an initial binding explicitly set in the shader?
> + *
> + * If so, constant_value contains an integer ir_constant representing
> the
> + * initial binding point.
> + */
> + unsigned explicit_binding:1;
> +
> + /**
> * Does this variable have an initializer?
> *
> * This is used by the linker to cross-validiate initializers of global
> diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp
> index 5b42935..535f5ce 100644
> --- a/src/glsl/ir_clone.cpp
> +++ b/src/glsl/ir_clone.cpp
> @@ -55,6 +55,7 @@ ir_variable::clone(void *mem_ctx, struct hash_table *ht)
> const
> var->pixel_center_integer = this->pixel_center_integer;
> var->explicit_location = this->explicit_location;
> var->explicit_index = this->explicit_index;
> + var->explicit_binding = this->explicit_binding;
> var->has_initializer = this->has_initializer;
> var->depth_layout = this->depth_layout;
>
> --
> 1.8.3.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
