[Mesa-dev] [PATCH 05/11] glsl: Propagate explicit binding information from AST to IR.

[Paul Berry]

On 17 July 2013 18:24, Kenneth Graunke <kenneth at 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 <kenneth at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130718/90bdd17b/attachment.html>