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

[Anuj Phogat]

On Tue, Feb 18, 2014 at 5:28 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 02/18/2014 03:36 PM, Anuj Phogat wrote:
>> 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.
>
> I don't think that's right.  I think they should both produce an error.
>  The spec says, "All redeclarations of gl_FragCoord in all fragment
> shaders in a single program must have the same set of qualifiers."  I
> can't see any reason to give an error for case 2 but not for case 1.
>
I agree.
> We should also check NVIDIA.
>
NVIDIA driver produces compilation error in both cases. I'll add a check
to handle them correctly in mesa.