<div dir="ltr">On 4 September 2013 08:51, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On 09/03/2013 08:28 AM, Paul Berry wrote: > On 29 August 2013 09:05, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a> </div><div>> <mailto:<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>>> wrote: > > From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a> </div>> <mailto:<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>>> <div><div>> > 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. </div></div>The spec does say: <div> "If gl_Color is redeclared with an interpolation qualifier, then gl_FrontColor and gl_BackColor (if they are written to) must </div> 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. </blockquote><div> </div><div>Yeah, that one makes sense to me. But I'm wondering about silly corner cases like: </div><div>[vertex shader] </div><div>main() { gl_FrontColor = vec4(0); gl_Position = vec4(0); } </div><div>[fragment shader] </div><div>noperspective in vec4 gl_Color; /* no static read */ </div><div>main() { gl_FragColor = vec4(0); } </div><div> </div><div>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. </div><div>Or this: </div><div>[vertex shader] </div><div>noperspective out vec4 gl_FrontColor; /* no static write */ </div><div>main() { gl_Position = vec4(0); } </div><div>[fragment shader] </div> <div>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. </div><div> </div><div>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). </div> <div> </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div> > On Mesa, all of the tests that expect linking failures fail. Basically, > Mesa's linker accepts everything. > > NVIDIA (304.64 on GTX 260) fails the same set of tests. > > Signed-off-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a> </div>> <mailto:<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>>> <div><div>> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=47755" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=47755</a> > --- > > I haven't run this on AMD's driver yet. If someone would be so kind > as to: > > 1. Apply this patch. > > 2. make > > 3. Run the following bit of shell: > > for i in > generated_tests/spec/glsl-1.30/linker/interpolation-qualifiers/*.shader_test > do > echo $i > bin/shader_runner $i -auto -fbo > done > output.txt > > 4. Email output.txt to me. > > Thanks! > > generated_tests/CMakeLists.txt | 6 +- > .../interpolation-qualifier-built-in-variable.py | 390 > +++++++++++++++++++++ > 2 files changed, 395 insertions(+), 1 deletion(-) > create mode 100644 > generated_tests/interpolation-qualifier-built-in-variable.py > > diff --git a/generated_tests/CMakeLists.txt > b/generated_tests/CMakeLists.txt > index a0ba4b1..a4d85dc 100644 > --- a/generated_tests/CMakeLists.txt > +++ b/generated_tests/CMakeLists.txt > @@ -60,6 +60,9 @@ piglit_make_generated_tests( > piglit_make_generated_tests( > cl_store_tests.list > generate-cl-store-tests.py) > +piglit_make_generated_tests( > + interpolation-qualifier-built-in-variable.list > + interpolation-qualifier-built-in-variable.py) > > # Add a "gen-tests" target that can be used to generate all the > # tests without doing any other compilation. > @@ -73,4 +76,5 @@ add_custom_target(gen-tests ALL > non-lvalue_tests.list > texture_query_lod_tests.list > shader_bit_encoding_tests.list > - uniform-initializer_tests.list) > + uniform-initializer_tests.list > + interpolation-qualifier-built-in-variable.list) > diff --git > a/generated_tests/interpolation-qualifier-built-in-variable.py > b/generated_tests/interpolation-qualifier-built-in-variable.py > new file mode 100644 > index 0000000..d03ed4d > --- /dev/null > +++ b/generated_tests/interpolation-qualifier-built-in-variable.py > @@ -0,0 +1,390 @@ > +# coding=utf-8 > +# > +# Copyright © 2013 Intel Corporation > +# > +# Permission is hereby granted, free of charge, to any person > obtaining a > +# copy of this software and associated documentation files (the > "Software"), > +# to deal in the Software without restriction, including without > limitation > +# the rights to use, copy, modify, merge, publish, distribute, > sublicense, > +# and/or sell copies of the Software, and to permit persons to whom the > +# Software is furnished to do so, subject to the following conditions: > +# > +# The above copyright notice and this permission notice (including > the next > +# paragraph) shall be included in all copies or substantial > portions of the > +# Software. > +# > +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO > EVENT SHALL > +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > OR OTHER > +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING > +# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > +# DEALINGS IN THE SOFTWARE. > + > +import struct > +import os > +import os.path > +from mako.template import Template > +from textwrap import dedent > + > +interpolation_modes = { > + 'flat', > + 'noperspective', > + 'smooth', > + 'default' > +} > > > This uses Python's "set literal" syntax, which was introduced in Python > 2.7. Piglit only requires 2.6, so I think we should change this to a list: > > interpolation_modes = [ > 'flat', > 'noperspective', > 'smooth', > 'default' > ] > > Same with vertex_shader_variables and vertex_shader_variables_front_only. </div></div>Ah... okay. I'll fix that. <div> > All of my other comments (below) are minor, so with this fixed, the > patch is: > > Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a> </div>> <mailto:<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>>> <div><div>> > > + > +vertex_shader_variables = { > + 'gl_FrontColor', > + 'gl_BackColor', > + 'gl_FrontSecondaryColor', > + 'gl_BackSecondaryColor' > +} > + > +vertex_shader_variables_front_only = { > + 'gl_FrontColor', > + 'gl_FrontSecondaryColor', > +} > + > +vertex_shader_variables_other_side = { > + 'gl_FrontColor': 'gl_BackColor', > + 'gl_BackColor': 'gl_FrontColor', > + 'gl_FrontSecondaryColor': 'gl_BackSecondaryColor', > + 'gl_BackSecondaryColor': 'gl_FrontSecondaryColor' > +} > + > +fragment_shader_variables = { > + 'gl_FrontColor': 'gl_Color', > + 'gl_BackColor': 'gl_Color', > + 'gl_FrontSecondaryColor': 'gl_SecondaryColor', > + 'gl_BackSecondaryColor': 'gl_SecondaryColor' > +} > > > Minor quibble: vertex_shader_variables_other_side and > fragment_shader_variables sound like names of lists, when they're in > fact mappings from one variable name to another. Maybe rename to > something like "other_side_map" and "vs_to_fs_map"? </div></div>That's fair. <div><div> > + > +template = Template(dedent("""\ > + # 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." > + # > + # Even though some of the other rules for interpolation qualifier > + # matching changed in 4.x specifications, this rule has > remained the > + # same. > + [require] > + GLSL >= 1.30 > + > + [vertex shader] > + % if vs_mode != 'default': > + ${vs_mode} out vec4 ${vs_variable}; > + % endif > + void main() { gl_Position = vec4(0); ${vs_variable} = vec4(0); } > + > + [fragment shader] > + % if fs_mode != 'default': > + ${fs_mode} in vec4 ${fs_variable}; > + % endif > + out vec4 c; > + void main() { c = ${fs_variable}; } > + > + [test] > + link error > + """)) > + > +for fs_mode in interpolation_modes: > + for vs_mode in interpolation_modes: > + if vs_mode == fs_mode: > + continue > + > + for var in vertex_shader_variables: > + filename = os.path.join('spec', > + 'glsl-1.30', > + 'linker', > + 'interpolation-qualifiers', > + > '{0}-{1}-{2}-{3}.shader_test'.format(vs_mode, > + > var, > + > fs_mode, > + > fragment_shader_variables[var])) > + print filename > + > + dirname = os.path.dirname(filename) > + if not os.path.exists(dirname): > + os.makedirs(dirname) > + > + f = open(filename, 'w') > + f.write(template.render(vs_mode=vs_mode, > + vs_variable=var, > + fs_mode=fs_mode, > + > fs_variable=fragment_shader_variables[var])) > + f.close() > + > +template = Template(dedent("""\ > + # 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." > + # > + # Even though some of the other rules for interpolation qualifier > + # matching changed in 4.x specifications, this rule has > remained the > + # same. > + # > + # We interpret the sentence "variables that are statically used > within the > + # shaders in a program" to mean static use of the variable in a > shader stage > + # invokes the redeclaration requirement for that stage only. > This is based on > + # the additional text "..gl_FrontColor and gl_BackColor (if > they are written > + # to) must also be redeclared with the same interpolation > qualifier..." > + [require] > + GLSL >= 1.30 > + > + [vertex shader] > + ${vs_mode} out vec4 ${vs_variable}; > + void main() { gl_Position = vec4(0); ${vs_variable} = vec4(0); } > + > + [fragment shader] > + out vec4 c; > + void main() { c = vec4(0); } > + > + [test] > + link success > + """)) > + > +for vs_mode in interpolation_modes: > + if vs_mode == 'default': > + continue > + > + for var in vertex_shader_variables: > + filename = os.path.join('spec', > + 'glsl-1.30', > + 'linker', > + 'interpolation-qualifiers', > + > '{0}-{1}-unused-{2}.shader_test'.format(vs_mode, > + > var, > + > fragment_shader_variables[var])) > + print filename > + > + dirname = os.path.dirname(filename) > + if not os.path.exists(dirname): > + os.makedirs(dirname) > + > + f = open(filename, 'w') > + f.write(template.render(vs_mode=vs_mode, > + vs_variable=var)) > + f.close() > + > +template = Template(dedent("""\ > + # 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." > + # > + # Even though some of the other rules for interpolation qualifier > + # matching changed in 4.x specifications, this rule has > remained the > + # same. > + # > + # We interpret the sentence "variables that are statically used > within the > + # shaders in a program" to mean static use of the variable in a > shader stage > + # invokes the redeclaration requirement for that stage only. > This is based on > + # the additional text "..gl_FrontColor and gl_BackColor (if > they are written > + # to) must also be redeclared with the same interpolation > qualifier..." > + [require] > + GLSL >= 1.30 > + > + [vertex shader] > + void main() { gl_Position = vec4(0); } > + > + [fragment shader] > + ${fs_mode} in vec4 ${fs_variable}; > + out vec4 c; > + void main() { c = ${fs_variable}; } > + > + [test] > + link success > + """)) > + > +for fs_mode in interpolation_modes: > + if fs_mode == 'default': > + continue > + > + for var in vertex_shader_variables_front_only: > + filename = os.path.join('spec', > + 'glsl-1.30', > + 'linker', > + 'interpolation-qualifiers', > + > 'unused-{0}-{1}-{2}.shader_test'.format(var, > + > fs_mode, > + > fragment_shader_variables[var])) > + print filename > + > + dirname = os.path.dirname(filename) > + if not os.path.exists(dirname): > + os.makedirs(dirname) > + > + f = open(filename, 'w') > + f.write(template.render(vs_mode=vs_mode, > + vs_variable=var, > + fs_mode=fs_mode, > + > fs_variable=fragment_shader_variables[var])) > + f.close() > + > + > +template = Template(dedent("""\ > + # 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." > + # > + # Even though some of the other rules for interpolation qualifier > + # matching changed in 4.x specifications, this rule has > remained the > + # same. > + # > + # We interpret the sentence "variables that are statically used > within the > + # shaders in a program" to mean static use of the variable in > any shader in > + # the program invokes the redeclaration requirement. Since > neither shader > + # accesses any built-in variables, linking should succeed no > matter what the > + # interpolation qualifiers say. > + [require] > + GLSL >= 1.30 > + > + [vertex shader] > + % if vs_mode != 'default': > + ${vs_mode} out vec4 ${vs_variable}; > + % endif > + void main() { gl_Position = vec4(0); } > + > + [fragment shader] > + % if fs_mode != 'default': > + ${fs_mode} in vec4 ${fs_variable}; > + % endif > + out vec4 c; > + void main() { c = vec4(0); } > + > + [test] > + link success > + """)) > + > +for fs_mode in interpolation_modes: > + for vs_mode in interpolation_modes: > + if vs_mode == fs_mode: > + continue > + > + for var in vertex_shader_variables: > + filename = os.path.join('spec', > + 'glsl-1.30', > + 'linker', > + 'interpolation-qualifiers', > + > 'unused-{0}-{1}-unused-{2}-{3}.shader_test'.format(vs_mode, > + > var, > + > fs_mode, > + > fragment_shader_variables[var])) > + print filename > + > + dirname = os.path.dirname(filename) > + if not os.path.exists(dirname): > + os.makedirs(dirname) > + > + f = open(filename, 'w') > + f.write(template.render(vs_mode=vs_mode, > + vs_variable=var, > + fs_mode=fs_mode, > + > fs_variable=fragment_shader_variables[var])) > + f.close() > + > + > +template = Template(dedent("""\ > + # 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." > + # > + # Even though some of the other rules for interpolation qualifier > + # matching changed in 4.x specifications, this rule has > remained the > + # same. > + [require] > + GLSL >= 1.30 > + > + [vertex shader] > + % if fs_mode != 'default': > + ${fs_mode} out vec4 ${front_variable}; > + % endif > + % if vs_mode != 'default': > + ${vs_mode} out vec4 ${back_variable}; > > > It seems a little misleading to call these "front_variable" and > "back_variable" when in fact sometimes the former is the back variable > and the latter is the front variable. How about just "vs_variable_1" > and "vs_variable_2"? </div></div>Yeah... how do this_side_variable and other_side_variable sound? </blockquote><div> </div><div>Sounds good to me. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div> > + % endif > + void main() { gl_Position = vec4(0); ${front_variable} = > vec4(0); ${back_variable} = vec4(1); } > + > + [fragment shader] > + % if fs_mode != 'default': > + ${fs_mode} in vec4 ${fs_variable}; > + % endif > + out vec4 c; > + void main() { c = ${fs_variable}; } > + > + [test] > + link error > + """)) > + > +for fs_mode in interpolation_modes: > + for vs_mode in interpolation_modes: > + if vs_mode == fs_mode: > + continue > + > + for var in vertex_shader_variables: > + var_back = vertex_shader_variables_other_side[var] > > > Similar comment here: "var_back" is a misleading name. </div>var => this_side, var_back => other_side? </blockquote><div> </div><div>I like it. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + filename = os.path.join('spec', > + 'glsl-1.30', > + 'linker', > + 'interpolation-qualifiers', > + > '{0}-{1}-{2}-{3}.shader_test'.format(vs_mode, > + > var, > + > fs_mode, > + > var_back)) > + print filename > + > + dirname = os.path.dirname(filename) > + if not os.path.exists(dirname): > + os.makedirs(dirname) > + > + f = open(filename, 'w') > + f.write(template.render(vs_mode=vs_mode, > + front_variable=var, > + back_variable=var_back, > + fs_mode=fs_mode, > + > fs_variable=fragment_shader_variables[var])) > + f.close() > -- > 1.8.1.4 > > _______________________________________________ > Piglit mailing list </div></div>> <a href="mailto:Piglit@lists.freedesktop.org" target="_blank">Piglit@lists.freedesktop.org</a> <mailto:<a href="mailto:Piglit@lists.freedesktop.org" target="_blank">Piglit@lists.freedesktop.org</a>> > <a href="http://lists.freedesktop.org/mailman/listinfo/piglit" target="_blank">http://lists.freedesktop.org/mailman/listinfo/piglit</a> </blockquote></div> </div></div>