[Mesa-dev] [PATCH 1/2] glsl: Fix redeclaration rules for gl_FragCoord.

[Anuj Phogat]

On Thu, Apr 17, 2014 at 1:05 AM, Chris Forbes <chrisf at ijw.co.nz> wrote:
> Correct, this doesn't try to tackle the link-time cases at all.
>
> After sending these out, Anuj told me that he had similar patches
> pending review. Unfortunate to duplicate effort, but it's what I get
> for doing a bunch of random bug-stomping, I guess :)
>
> Hopefully we can pick the best ideas from both series.
>
Chris, my series got r-b Ian. It'll be extra work to decouple compiler and
linker patches in the series and then testing. So, I'm inclined towards
landing my series as it is. Hope that is fine with you. We can always
improve the compiler code later if required.

> On Thu, Apr 17, 2014 at 6:29 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
>> On 04/12/2014 10:30 PM, Chris Forbes wrote:
>>> The ARB_fragment_coord_conventions spec, section 4.3.x (Input Layout
>>> Qualifiers) says:
>>>
>>>   "All redeclarations of gl_FragCoord in all fragment shaders in a
>>>   single program must have the same set of qualifiers. Within any
>>>   shader, the first redeclarations of gl_FragCoord must appear before
>>>   any use of gl_FragCoord."
>>>
>>> Fixes piglit's tests:
>>>    arb_fragment_coord_conventions/compiler/redeclaration-after-use
>>>    arb_fragment_coord_conventions/compiler/redeclaration-around-use
>>>    glsl-1.50/compiler/fragment_coord_conventions/layout-qualifiers-conflicting-case-1
>>>    glsl-1.50/compiler/fragment_coord_conventions/layout-qualifiers-conflicting-case-2
>>>
>>> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>>> ---
>>>  src/glsl/ast_to_hir.cpp | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>>> index 9b9d511..d1eb3e2 100644
>>> --- a/src/glsl/ast_to_hir.cpp
>>> +++ b/src/glsl/ast_to_hir.cpp
>>> @@ -2715,9 +2715,29 @@ get_variable_being_redeclared(ir_variable *var, YYLTYPE loc,
>>>        /* Allow redeclaration of gl_FragCoord for ARB_fcc layout
>>>         * qualifiers.
>>>         */
>>> +
>>> +      if (earlier->data.how_declared != ir_var_declared_implicitly
>>> +          && (earlier->data.origin_upper_left != var->data.origin_upper_left
>>> +             || earlier->data.pixel_center_integer != var->data.pixel_center_integer)) {
>>> +         _mesa_glsl_error(&loc, state,
>>> +                          "Inconsistent redeclarations of gl_FragCoord");
>>> +      }
>>> +
>>>        earlier->data.origin_upper_left = var->data.origin_upper_left;
>>>        earlier->data.pixel_center_integer = var->data.pixel_center_integer;
>>>
>>> +      if (earlier->data.used &&
>>> +          earlier->data.how_declared == ir_var_declared_implicitly) {
>>> +         /* ARB_fragment_coord_conventions spec Section 4.3.x.1
>>> +          * (Input Layout Qualifier) says:
>>> +          *
>>> +          * "Within any shader, the first redeclarations of gl_FragCoord
>>> +          * must appear before any use of gl_FragCoord."
>>> +          */
>>> +         _mesa_glsl_error(&loc, state,
>>> +                          "First redeclaration of gl_FragCoord must appear before any use");
>>> +      }
>>> +
>>>        /* According to section 4.3.7 of the GLSL 1.30 spec,
>>>         * the following built-in varaibles can be redeclared with an
>>>         * interpolation qualifier:
>>>
>>
>> It seems like this will catch multiple, inconsistent redeclarations
>> within a single fragment shader...but the spec text quoted indicates
>> that redeclarations in all fragment shaders being linked together into a
>> single program need to be consistent.
>>
>> I don't see any code to handle that in linker*.cpp, so presumably we
>> ought to add that as well.  Regardless, this is a good improvement!
>>
>> Both patches are:
>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>>
>> Thanks, Chris.
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev