[Mesa-dev] [PATCH] glsl: trap atomic operations on illegal image formats in compiler.

[Matt Turner]

On Thu, May 5, 2016 at 3:19 PM, Dave Airlie <airlied at gmail.com> wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> This fixes:
> GL43-CTS.shader_image_load_store.negative-compileErrors
>
> where shader 9 was being compiled with atomic operation on an r16i.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  src/compiler/glsl/ast_function.cpp      | 21 +++++++++++++++++++++
>  src/compiler/glsl/builtin_functions.cpp |  6 +++---
>  src/compiler/glsl/glsl_parser_extras.h  |  7 +++++++
>  src/compiler/glsl/ir.h                  |  2 ++
>  4 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
> index fe0df15..3ea6e05 100644
> --- a/src/compiler/glsl/ast_function.cpp
> +++ b/src/compiler/glsl/ast_function.cpp
> @@ -139,6 +139,27 @@ verify_image_parameter(YYLTYPE *loc, _mesa_glsl_parse_state *state,
>        return false;
>     }
>
> +   if (formal->data.image_atomic) {
> +     if (actual->data.image_format != GL_R32UI &&
> +        actual->data.image_format != GL_R32I) {
> +       _mesa_glsl_error(loc, state,
> +                       "atomic operations can only happen on r32ui/r32i formats.");

Bunch of tabs in this patch.

> +       return false;
> +     }
> +   }
> +
> +   if (formal->data.image_atomic_exchange) {
> +     if ((actual->data.image_format != GL_R32UI &&
> +         actual->data.image_format != GL_R32I)) {
> +       /* check for the r32f special case. */
> +       if (!(state->has_shader_image_atomic_exchange_float() &&
> +            actual->data.image_format == GL_R32F)) {
> +        _mesa_glsl_error(loc, state,
> +                         "atomic exchange operations can only happen on r32ui/r32i formats (or r32f in GLES).");

Probably would be nice to line wrap this.

> +        return false;
> +       }
> +     }
> +   }
>     return true;
>  }
>
> diff --git a/src/compiler/glsl/builtin_functions.cpp b/src/compiler/glsl/builtin_functions.cpp
> index 25d914d..9d029bb 100644
> --- a/src/compiler/glsl/builtin_functions.cpp
> +++ b/src/compiler/glsl/builtin_functions.cpp
> @@ -478,9 +478,7 @@ shader_image_atomic(const _mesa_glsl_parse_state *state)
>  static bool
>  shader_image_atomic_exchange_float(const _mesa_glsl_parse_state *state)
>  {
> -   return (state->is_version(450, 320) ||
> -           state->ARB_ES3_1_compatibility_enable ||
> -           state->OES_shader_image_atomic_enable);
> +  return (state->has_shader_image_atomic_exchange_float());
>  }
>
>  static bool
> @@ -5436,6 +5434,8 @@ builtin_builder::_image_prototype(const glsl_type *image_type,
>     image->data.image_coherent = true;
>     image->data.image_volatile = true;
>     image->data.image_restrict = true;
> +   image->data.image_atomic = (flags & IMAGE_FUNCTION_AVAIL_ATOMIC) != 0;
> +   image->data.image_atomic_exchange = (flags & IMAGE_FUNCTION_AVAIL_ATOMIC_EXCHANGE) != 0;
>
>     return sig;
>  }
> diff --git a/src/compiler/glsl/glsl_parser_extras.h b/src/compiler/glsl/glsl_parser_extras.h
> index 7018347..9bf9245 100644
> --- a/src/compiler/glsl/glsl_parser_extras.h
> +++ b/src/compiler/glsl/glsl_parser_extras.h
> @@ -270,6 +270,13 @@ struct _mesa_glsl_parse_state {
>        return OES_geometry_shader_enable || is_version(150, 320);
>     }
>
> +   bool has_shader_image_atomic_exchange_float() const
> +   {
> +      return (is_version(450, 320) ||
> +             ARB_ES3_1_compatibility_enable ||
> +             OES_shader_image_atomic_enable);
> +   }
> +
>     void process_version_directive(YYLTYPE *locp, int version,
>                                    const char *ident);
>
> diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
> index 0c319ea..2bf04e68 100644
> --- a/src/compiler/glsl/ir.h
> +++ b/src/compiler/glsl/ir.h
> @@ -811,6 +811,8 @@ public:
>        unsigned image_volatile:1;
>        unsigned image_restrict:1;
>
> +      unsigned image_atomic:1;
> +      unsigned image_atomic_exchange:1;

Leave a newline after these.

>        /**
>         * ARB_shader_storage_buffer_object
>         */
> --