[Mesa-dev] [PATCH V4 1/4] glsl: Compile error if fs defines conflicting qualifiers for gl_FragCoord

[Anuj Phogat]

Ian, I posted the reworked patches after incorporating your comments.
Do you have any more comments on this series? It would be nice to land
these before upcoming release.


On Mon, Mar 17, 2014 at 11:37 AM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> GLSL 1.50 spec says:
>    "If gl_FragCoord is redeclared in any fragment shader in a program,
>     it must be redeclared in all the fragment shaders in that
>     program that have a static use gl_FragCoord. All redeclarations of
>     gl_FragCoord in all fragment shaders in a single program must
>     have the same set of qualifiers."
>
> This patch makes the glsl compiler to generate an error if we have a
> fragment shader defined with conflicting layout qualifier declarations
> for gl_FragCoord. For example:
>
> layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
> layout(pixel_center_integer) in vec4 gl_FragCoord;
>
> void main()
> {
> }
>
> V2: Some code refactoring for better readability.
>     Add compiler error conditions for redeclarations like:
>
> layout(origin_upper_left) in vec4 gl_FragCoord;
> layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
>
> and
>
> in vec4 gl_FragCoord;
> layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
>
> V3: Simplify function is_conflicting_fragcoord_redeclaration()
> V4: Check for null pointer before doing strcmp(var->name, "gl_FragCoord").
>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> Cc: <mesa-stable at lists.freedesktop.org>
> ---
>  src/glsl/ast_to_hir.cpp         | 58 +++++++++++++++++++++++++++++++++++++++++
>  src/glsl/glsl_parser_extras.cpp |  5 ++++
>  src/glsl/glsl_parser_extras.h   | 12 +++++++++
>  3 files changed, 75 insertions(+)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 8f6e901..b241ccf 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2298,6 +2298,34 @@ apply_image_qualifier_to_variable(const struct ast_type_qualifier *qual,
>     }
>  }
>
> +static inline const char*
> +get_layout_qualifier_string(bool origin_upper_left, bool pixel_center_integer)
> +{
> +   if (origin_upper_left && pixel_center_integer)
> +      return "origin_upper_left, pixel_center_integer";
> +   else if (origin_upper_left)
> +      return "origin_upper_left";
> +   else if (pixel_center_integer)
> +      return "pixel_center_integer";
> +   else
> +      return " ";
> +}
> +
> +static inline bool
> +is_conflicting_fragcoord_redeclaration(struct _mesa_glsl_parse_state *state,
> +                                       const struct ast_type_qualifier *qual)
> +{
> +   /* If gl_FragCoord was previously declared, and the qualifiers were
> +    * different in any way, return true.
> +    */
> +   if (state->fs_redeclares_gl_fragcoord) {
> +      return (state->fs_pixel_center_integer != qual->flags.q.pixel_center_integer
> +         || state->fs_origin_upper_left != qual->flags.q.origin_upper_left);
> +   }
> +
> +   return false;
> +}
> +
>  static void
>  apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>                                  ir_variable *var,
> @@ -2462,6 +2490,36 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>                        qual_string);
>     }
>
> +   if (var->name != NULL && strcmp(var->name, "gl_FragCoord") == 0) {
> +
> +      /* Make sure all gl_FragCoord redeclarations specify the same layout
> +       * qualifiers.
> +       */
> +      if (is_conflicting_fragcoord_redeclaration(state, qual)) {
> +         const char *const qual_string =
> +            get_layout_qualifier_string(qual->flags.q.origin_upper_left,
> +                                        qual->flags.q.pixel_center_integer);
> +
> +         const char *const state_string =
> +            get_layout_qualifier_string(state->fs_origin_upper_left,
> +                                        state->fs_pixel_center_integer);
> +
> +         _mesa_glsl_error(loc, state,
> +                          "gl_FragCoord redeclared with different layout "
> +                          "qualifiers (%s) and (%s) ",
> +                          state_string,
> +                          qual_string);
> +      }
> +      state->fs_origin_upper_left = qual->flags.q.origin_upper_left;
> +      state->fs_pixel_center_integer = qual->flags.q.pixel_center_integer;
> +      state->fs_redeclares_gl_fragcoord_with_no_layout_qualifiers =
> +         !qual->flags.q.origin_upper_left && !qual->flags.q.pixel_center_integer;
> +      state->fs_redeclares_gl_fragcoord =
> +         state->fs_origin_upper_left ||
> +         state->fs_pixel_center_integer ||
> +         state->fs_redeclares_gl_fragcoord_with_no_layout_qualifiers;
> +   }
> +
>     if (qual->flags.q.explicit_location) {
>        validate_explicit_location(qual, var, state, loc);
>     } else if (qual->flags.q.explicit_index) {
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index 61ae621..340b128 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -201,6 +201,11 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>     this->default_uniform_qualifier->flags.q.shared = 1;
>     this->default_uniform_qualifier->flags.q.column_major = 1;
>
> +   this->fs_redeclares_gl_fragcoord = false;
> +   this->fs_origin_upper_left = false;
> +   this->fs_pixel_center_integer = false;
> +   this->fs_redeclares_gl_fragcoord_with_no_layout_qualifiers = false;
> +
>     this->gs_input_prim_type_specified = false;
>     this->gs_input_size = 0;
>     this->in_qualifier = new(this) ast_type_qualifier();
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index 300d900..2017913 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -204,6 +204,18 @@ struct _mesa_glsl_parse_state {
>     struct ast_type_qualifier *default_uniform_qualifier;
>
>     /**
> +    * Variables to track different cases if a fragment shader redeclares
> +    * built-in variable gl_FragCoord.
> +    *
> +    * Note: These values are computed at ast_to_hir time rather than at parse
> +    * time.
> +    */
> +   bool fs_redeclares_gl_fragcoord;
> +   bool fs_origin_upper_left;
> +   bool fs_pixel_center_integer;
> +   bool fs_redeclares_gl_fragcoord_with_no_layout_qualifiers;
> +
> +   /**
>      * True if a geometry shader input primitive type was specified using a
>      * layout directive.
>      *
> --
> 1.8.3.1
>