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

[Ian Romanick]

On 02/25/2014 01:39 PM, Anuj Phogat wrote:
> On Tue, Feb 25, 2014 at 7:43 AM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 02/24/2014 05:34 PM, Anuj Phogat 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;
>>>
>>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>>> Cc: <mesa-stable at lists.freedesktop.org>
>>> ---
>>> This series is also available at:
>>> https://github.com/aphogat/mesa.git, branch='review'
>>>
>>>    src/glsl/ast_to_hir.cpp         | 60 +++++++++++++++++++++++++++++++++++++++++
>>>    src/glsl/glsl_parser_extras.cpp |  5 ++++
>>>    src/glsl/glsl_parser_extras.h   | 12 +++++++++
>>>    3 files changed, 77 insertions(+)
>>>
>>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>>> index f06baeb..9fe3095 100644
>>> --- a/src/glsl/ast_to_hir.cpp
>>> +++ b/src/glsl/ast_to_hir.cpp
>>> @@ -2295,6 +2295,36 @@ 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)
>>
>> state can also be const.
>>
>>> +{
>>> +   bool conflicting_origin_upper_left =
>>> +      (state->fs_origin_upper_left && !qual->flags.q.origin_upper_left) ||
>>> +      (!state->fs_origin_upper_left && qual->flags.q.origin_upper_left &&
>>> +       state->fs_redeclares_gl_fragcoord);
>>> +
>>> +   bool conflicting_pixel_center_integer =
>>> +      (state->fs_pixel_center_integer && !qual->flags.q.pixel_center_integer) ||
>>> +      (!state->fs_pixel_center_integer && qual->flags.q.pixel_center_integer &&
>>> +       state->fs_redeclares_gl_fragcoord);
>>
>> Does this condition also work?  It seems easier to understand.
>>
>>     /* If gl_FragCoord was previously declared, and the qualifiers were
>>      * different in any way, return false.
>>      */
>>     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 true;
> I think you meant 'return false' here? It works after making this change in last
> statement. Using '!=' made the function look simpler. Thanks for suggestion.

Yes.  When I wrote that line I was thinking the function returned true 
for valid.

>>> +
>>> +   return (conflicting_origin_upper_left || conflicting_pixel_center_integer);
>>> +}
>>> +
>>>    static void
>>>    apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>>>                                 ir_variable *var,
>>> @@ -2459,6 +2489,36 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>>>                       qual_string);
>>>       }
>>>
>>> +   if (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 d7f5202..fcbbf44 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.
>>>        *
>>>