[Piglit] [PATCH] glsl-1.30: Test interpolation qualifiers on built-in variables

[Ian Romanick]

On 09/04/2013 11:49 AM, Paul Berry wrote:
> On 4 September 2013 08:51, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
> 
>     On 09/03/2013 08:28 AM, Paul Berry wrote:
>     > On 29 August 2013 09:05, Ian Romanick <idr at freedesktop.org
>     <mailto:idr at freedesktop.org>
>     > <mailto:idr at freedesktop.org <mailto:idr at freedesktop.org>>> wrote:
>     >
>     >     From: Ian Romanick <ian.d.romanick at intel.com
>     <mailto:ian.d.romanick at intel.com>
>     >     <mailto:ian.d.romanick at intel.com
>     <mailto:ian.d.romanick at intel.com>>>
>     >
>     >     Section 4.3.7 (Interpolation) of the GLSL 1.30 spec says:
>     >
>     >         "If gl_Color is redeclared with an interpolation
>     qualifier, then
>     >         gl_FrontColor and gl_BackColor (if they are written to)
>     must also be
>     >         redeclared with the same interpolation qualifier, and vice
>     versa. If
>     >         gl_SecondaryColor is redeclared with an interpolation
>     qualifier,
>     >         then gl_FrontSecondaryColor and gl_BackSecondaryColor (if
>     they are
>     >         written to) must also be redeclared with the same
>     interpolation
>     >         qualifier, and vice versa. This qualifier matching on
>     predeclared
>     >         variables is only required for variables that are
>     statically used
>     >         within the shaders in a program."
>     >
>     >     This adds five sets of tests from a single generator script:
>     >
>     >         - Set an interpolation qualifier, possibly the default,
>     for one of
>     >           gl_FrontColor, gl_BackColor, gl_FrontSecondaryColor, or
>     >           gl_BackSecondaryColor in the vertex shader, and set a
>     different
>     >           interpolation qualifier, possibly the default, for the
>     matching
>     >           gl_Color or gl_SecondaryColor in the fragment shader.
>      This should
>     >           fail to link.
>     >
>     >         - Set a non-default interpolation qualifier for one of
>     >           gl_FrontColor, gl_BackColor, gl_FrontSecondaryColor, or
>     >           gl_BackSecondaryColor in the vertex shader, and have no
>     >           redeclaration in the fragment shader.  In the fragment
>     shader,
>     >           neither gl_Color nor gl_SecondaryColor is used.  This should
>     >           successfully link.
>     >
>     >         - Set a non-default interpolation qualifier for one of
>     gl_Color or
>     >           gl_SecondaryColor in the fragment shader, and have no
>     >           redeclaration in the vertex shader.  In the vertex
>     shader, none of
>     >           the built-in color variables are used.  This should
>     successfully
>     >           link.
>     >
>     >         - Set an interpolation qualifier, possibly the default,
>     for one of
>     >           gl_FrontColor, gl_BackColor, gl_FrontSecondaryColor, or
>     >           gl_BackSecondaryColor in the vertex shader, and set a
>     different
>     >           interpolation qualifier, possibly the default, for the
>     matching
>     >           gl_Color or gl_SecondaryColor in the fragment shader.
>      Neither
>     >           stage uses any of the color variables.  This should
>     successfully
>     >           to link.
>     >
>     >         - Set conflicting interpolation qualifiers for gl_FrontColor /
>     >           gl_BackColor or gl_FrontSecondaryColor /
>     gl_BackSecondaryColor in
>     >           the vertex shader, and set a matching interpolation
>     qualifier on
>     >           gl_Color or gl_SecondaryColor in the fragment shader for
>     one of
>     >           the vertex shader built-in varaibles.  This should fail
>     to link.
>     >
>     >
>     > Do we care about testing variants where the variable is statically
>     > accessed in the VS but not in the FS (or vice versa)?  It's not
>     entirely
>     > clear to me what the spec says about these situations.
> 
>     The spec does say:
> 
>         "If gl_Color is redeclared with an interpolation qualifier, then
>         gl_FrontColor and gl_BackColor (if they are written to) must
>         also be redeclared with the same interpolation qualifier..."
> 
>     So, I'm inferring that if there is no static write to either
>     gl_FrontColor or gl_BackColor in the vertex shader, then the vertex
>     shader does not need a redeclaration no matter what the fragment shader
>     does.
> 
> 
> Yeah, that one makes sense to me.  But I'm wondering about silly corner
> cases like:
> 
> [vertex shader]
> main() { gl_FrontColor = vec4(0); gl_Position = vec4(0); }
> 
> [fragment shader]
> noperspective in vec4 gl_Color; /* no static read */
> main() { gl_FragColor = vec4(0); }
> 
> By the letter of the spec, this should fail to link (since the spec
> doesn't make any mention of the possibility that gl_Color isn't read
> from), but it's not obvious to me that this is the spec writers' intent.

This bit of text that I elided above was "and vice versa."  So the full
text reads:

    "If gl_Color is redeclared with an interpolation qualifier, then
    gl_FrontColor and gl_BackColor (if they are written to) must
    also be redeclared with the same interpolation qualifier, and vice
    versa."

I interpreted "vice versa" to mean:

    If gl_FrontColor or gl_BackColor is redeclared with an
    interpolation qualifier, then gl_Color (if it is read) must
    also be redeclared with the same interpolation qualifier.

I added these test cases because my initial fix for the cases where both
stages use color variables broke these cases.

> Or this:
> 
> [vertex shader]
> noperspective out vec4 gl_FrontColor; /* no static write */
> main() { gl_Position = vec4(0); }
> 
> [fragment shader]
> main() { gl_FragColor = gl_Color; }
> 
> If we consider "and vice versa" to mean "if gl_FrontColor or
> gl_BackColor is redeclared with an interpolation qualifier, then
> gl_Color (if it is read from) must also be redeclared with the same
> interpolation qualifier", then by the letter of the spec, this should
> fail to link too, but again I'm not certain whether that's the spec
> writers' intent.
> 
> 
> Of course, it seems unlikely that a sane programmer would ever write
> shaders like these, so I'm not sure how much we care (cue joke about the
> sanity of typical GLSL programmers).

I can imagine cases where a generic vertex shader writes a bunch of
stuff that some of the fragment shaders with which it links do not use.
 There's also a lot of GLSL code generated by other software.  I can
easily imagine a simple generator creating some rubbish like this.