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

[Anuj Phogat]

On Tue, Feb 18, 2014 at 11:01 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 02/10/2014 05:29 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()
>> {
>>    gl_FragColor = gl_FragCoord.xyzz;
>> }
>>
>> Cc: <mesa-stable at lists.freedesktop.org>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> ---
>>  src/glsl/ast_to_hir.cpp         | 39 +++++++++++++++++++++++++++++++++++++++
>>  src/glsl/glsl_parser_extras.cpp |  3 +++
>>  src/glsl/glsl_parser_extras.h   | 10 ++++++++++
>>  3 files changed, 52 insertions(+)
>>
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index c89a26b..7d7d89b 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -2374,6 +2374,45 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>>                      qual_string);
>>     }
>>
>> +   /* Make sure all gl_FragCoord redeclarations specify the same layout
>> +    * qualifier type.
>> +    */
>> +   bool conflicting_pixel_center_integer =
>> +      state->fs_pixel_center_integer &&
>> +      !qual->flags.q.pixel_center_integer;
>> +
>> +   bool conflicting_origin_upper_left =
>> +      state->fs_origin_upper_left &&
>> +      !qual->flags.q.origin_upper_left;
>
> I don't think this catches all the cases.  What about
>
> layout(origin_upper_left                      ) in vec4 gl_FragCoord;
> layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
>
Nice catch. I'll update my patch to include this case. What do you think
about following two cases?
case 1:
in vec4 gl_FragCoord;
layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;

AMD produces no compilation error. This patch matches the behavior.

case 2:
layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
in vec4 gl_FragCoord;

AMD produces compilation error. This patch matches the behavior.

>> +
>> +   if (conflicting_pixel_center_integer || conflicting_origin_upper_left) {
>> +      const char *const qual_string =
>> +         (conflicting_pixel_center_integer && conflicting_origin_upper_left) ?
>> +         "pixel_center_integer, origin_upper_left" :
>> +         (conflicting_pixel_center_integer ?
>> +         "origin_upper_left" : "pixel_center_integer");
>> +
>> +      const char *const state_string =
>> +         (state->fs_pixel_center_integer &&
>> +          state->fs_origin_upper_left) ?
>> +         "pixel_center_integer, origin_upper_left" :
>> +         (state->fs_origin_upper_left ?
>> +         "origin_upper_left" : "pixel_center_integer");
>> +
>> +      _mesa_glsl_error(loc, state,
>> +                       "different layout qualifiers were specified with "
>> +                       "`gl_FragCoord' (%s) and (%s) "
>> +                       "redeclare fragment shader input `gl_FragCoord'",
>> +                       state_string,
>> +                       qual_string);
>> +   }
>> +
>> +   if (qual->flags.q.pixel_center_integer)
>> +      state->fs_pixel_center_integer = true;
>> +
>> +   if (qual->flags.q.origin_upper_left)
>> +      state->fs_origin_upper_left = true;
>> +
>>     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 b822d22..b06337e 100644
>> --- a/src/glsl/glsl_parser_extras.cpp
>> +++ b/src/glsl/glsl_parser_extras.cpp
>> @@ -193,6 +193,9 @@ _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_origin_upper_left = false;
>> +   this->fs_pixel_center_integer = false;
>> +
>>     this->gs_input_prim_type_specified = false;
>>     this->gs_input_prim_type = GL_POINTS;
>>     this->gs_input_size = 0;
>> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
>> index 7d66147..674dae5 100644
>> --- a/src/glsl/glsl_parser_extras.h
>> +++ b/src/glsl/glsl_parser_extras.h
>> @@ -182,6 +182,16 @@ struct _mesa_glsl_parse_state {
>>     struct ast_type_qualifier *default_uniform_qualifier;
>>
>>     /**
>> +    * True if a fragment shader has an input layout for redeclaring the
>> +    * built-in variable gl_FragCoord.
>> +    *
>> +    * Note: this value is computed at ast_to_hir time rather than at parse
>> +    * time.
>> +    */
>> +   bool fs_origin_upper_left;
>> +   bool fs_pixel_center_integer;
>> +
>> +   /**
>>      * True if a geometry shader input primitive type was specified using a
>>      * layout directive.
>>      *
>>
>