[Mesa-dev] [PATCH] glsl: allow precision qualifier on sampler arrays

[Anuj Phogat]

On Fri, Aug 22, 2014 at 2:23 PM, Frank Henigman <fjhenigman at google.com> wrote:
> If a precision qualifer is allowed on type T, it should be allowed
> on an array of T.  Refactor the check to ensure this is the case.
> ---
> I wanted to expand the comment to say why is_record() is in there,
> but I don't understand why it is.  I thought structs couldn't have
> precision qualifiers.
Right. I noticed some error messages in src/glsl/ast_to_hir.cpp
supporting your argument:
"precision qualifiers do not apply to structures"
"precision qualifiers can't be applied to structures"

We have piglit tests for this which currently pass with Mesa master:
tests/spec/glsl-es-1.00/compiler/precision-qualifiers/precision-struct-01.frag
tests/spec/glsl-es-1.00/compiler/precision-qualifiers/precision-struct-02.frag

So, I think we can just omit is_record() entry from here.
>
>  src/glsl/ast_to_hir.cpp | 75 +++++++++++++++++++++++++++----------------------
>  1 file changed, 41 insertions(+), 34 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 30b02d0..998404a 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -3202,6 +3202,41 @@ validate_identifier(const char *identifier, YYLTYPE loc,
>     }
>  }
>
> +static bool
> +precision_qualifier_allowed(const glsl_type *type)
> +{
> +   /* Precision qualifiers apply to floating point, integer and sampler
> +    * types.
> +    *
> +    * Section 4.5.2 (Precision Qualifiers) of the GLSL 1.30 spec says:
> +    *    "Any floating point or any integer declaration can have the type
> +    *    preceded by one of these precision qualifiers [...] Literal
> +    *    constants do not have precision qualifiers. Neither do Boolean
> +    *    variables.
> +    *
> +    * Section 4.5 (Precision and Precision Qualifiers) of the GLSL 1.30
> +    * spec also says:
> +    *
> +    *     "Precision qualifiers are added for code portability with OpenGL
> +    *     ES, not for functionality. They have the same syntax as in OpenGL
> +    *     ES."
> +    *
> +    * Section 8 (Built-In Functions) of the GLSL ES 1.00 spec says:
> +    *
> +    *     "uniform lowp sampler2D sampler;
> +    *     highp vec2 coord;
> +    *     ...
> +    *     lowp vec4 col = texture2D (sampler, coord);
> +    *                                            // texture2D returns lowp"
> +    *
> +    * From this, we infer that GLSL 1.30 (and later) should allow precision
> +    * qualifiers on sampler types just like float and integer types.
> +    */
> +   return type->is_float()
> +       || type->is_integer()
> +       || type->is_record()
> +       || type->is_sampler();
> +}
>
>  ir_rvalue *
>  ast_declarator_list::hir(exec_list *instructions,
> @@ -3689,41 +3724,13 @@ ast_declarator_list::hir(exec_list *instructions,
>        }
>
>
> -      /* Precision qualifiers apply to floating point, integer and sampler
> -       * types.
> -       *
> -       * Section 4.5.2 (Precision Qualifiers) of the GLSL 1.30 spec says:
> -       *    "Any floating point or any integer declaration can have the type
> -       *    preceded by one of these precision qualifiers [...] Literal
> -       *    constants do not have precision qualifiers. Neither do Boolean
> -       *    variables.
> -       *
> -       * Section 4.5 (Precision and Precision Qualifiers) of the GLSL 1.30
> -       * spec also says:
> -       *
> -       *     "Precision qualifiers are added for code portability with OpenGL
> -       *     ES, not for functionality. They have the same syntax as in OpenGL
> -       *     ES."
> -       *
> -       * Section 8 (Built-In Functions) of the GLSL ES 1.00 spec says:
> -       *
> -       *     "uniform lowp sampler2D sampler;
> -       *     highp vec2 coord;
> -       *     ...
> -       *     lowp vec4 col = texture2D (sampler, coord);
> -       *                                            // texture2D returns lowp"
> -       *
> -       * From this, we infer that GLSL 1.30 (and later) should allow precision
> -       * qualifiers on sampler types just like float and integer types.
> +      /* If a precision qualifier is allowed on a type, it is allowed on
> +       * an array of that type.
>         */
> -      if (this->type->qualifier.precision != ast_precision_none
> -          && !var->type->is_float()
> -          && !var->type->is_integer()
> -          && !var->type->is_record()
> -          && !var->type->is_sampler()
> -          && !(var->type->is_array()
> -               && (var->type->fields.array->is_float()
> -                   || var->type->fields.array->is_integer()))) {
> +      if (!(this->type->qualifier.precision == ast_precision_none
> +          || precision_qualifier_allowed(var->type)
> +          || (var->type->is_array()
> +             && precision_qualifier_allowed(var->type->fields.array)))) {
>
>           _mesa_glsl_error(&loc, state,
>                            "precision qualifiers apply only to floating point"
> --
> 2.1.0.rc2.206.gedb03e5
>

I think it would be better to omit the is_record() check in a separate
patch.
This patch is: Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>