[Piglit] [PATCH V2] arb_shading_language_420pack: test multiple layout qualifiers on the same line

[Timothy Arceri]

On Thu, 2016-01-14 at 09:18 -0800, Matt Turner wrote:
> On Wed, Jan 13, 2016 at 9:45 PM, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > While the spec is taking about multiple layout qualifiers we
> > interpret that
> > to mean layout-qualifier-names can also occur multiple times within
> > a single layout qualifier.
> > 
> > In Section 4.4 (Layout Qualifiers) of the GLSL 4.40 spec it
> > clarifies this:
> > 
> >    "More than one layout qualifier may appear in a single
> > declaration.
> >    Additionally, the same layout-qualifier-name can occur multiple
> > times
> >    within a layout qualifier or across multiple layout qualifiers
> > in the
> >    same declaration"
> > 
> > The results from the Nvidia binary are a little all over the place
> > with these
> > tests, most of the tests with arb_shading_language_420pack enabled
> > fail but
> > if the GLSL version is changed to 4.20 they pass.
> > The only tests that pass with arb_shading_language_420pack enabled
> > are the
> > global tests but that seems to be because they do not detect the
> > duplicates
> > and fail the negative tests.
> > 
> > V2: fix version mismatch in location tests, and prefix variable
> > with out
> > in global tests
> > ---
> 
> You seem to be interpreting this as a clarification on the earlier
> spec text rather than as a behavioral change based on NVIDIA's
> acceptance when using #version 420. I don't necessarily agree --
> their
> drivers are known to accept out-of-spec shaders.
> 
> In fact (as I'm sure you know) ARB_enhanced_layouts, explicitly
> expands the paragraph cited to contain new text about
> layout-qualifier-names. 

I actually totally missed that, thanks.

> I think this indicates an actual behavioral
> change and not a clarification meant to be applied to earlier shader
> versions.

I spent most of my time searching the older specs trying to find
something that says duplicates were not allowed in the first place ...
I couldn't find anything it seems undefined and drivers default to not
allowed.

The only thing I could find were sections specificly allowing it,
Section 4.3.8.1 (Input Layout Qualifiers) in the 4.10 spec:

"Any or all of these identifiers may be specified one or more times in
a single input layout declaration. If primitive mode, vertex spacing,
or ordering is declared more than once in the tessellation evaluation
shaders of a program, all such declarations must use the same
identifier."

I guess from that you can assume they are not normally allowed.

Anyway I think enabling this for enhanced layouts makes the most sense
as per your suggestion. I'll update the tests and Mesa patch thanks for
the feedback.

> 
> I think the tests have value, but I think that they should be testing
> ARB_enhanced_layouts instead of 420pack.
> 
> >  ...e-qualifier-in-single-declaration-420-pack.vert | 22
> > +++++++++++++
> >  ...ualifier-in-single-declaration-no-420-pack.vert | 21
> > ++++++++++++
> >  ...n-qualifier-in-single-declaration-420-pack.vert | 33
> > +++++++++++++++++++
> >  ...ualifier-in-single-declaration-no-420-pack.vert | 32
> > ++++++++++++++++++
> >  ...iple-stream-in-single-declaration-420-pack.geom | 36
> > ++++++++++++++++++++
> >  ...e-stream-in-single-declaration-no-420-pack.geom | 35
> > ++++++++++++++++++++
> >  ...ream-in-single-global-declaration-420-pack.geom | 38
> > ++++++++++++++++++++++
> >  ...m-in-single-global-declaration-no-420-pack.geom | 37
> > +++++++++++++++++++++
> >  ...e-qualifier-in-single-declaration-420-pack.geom | 25
> > ++++++++++++++
> >  ...ualifier-in-single-declaration-no-420-pack.geom | 24
> > ++++++++++++++
> >  ...fier-in-single-global-declaration-420-pack.geom | 27
> > +++++++++++++++
> >  ...r-in-single-global-declaration-no-420-pack.geom | 37
> > +++++++++++++++++++++
> >  12 files changed, 367 insertions(+)
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-multiple-qualifier-in-single
> > -declaration-420-pack.vert
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-multiple-qualifier-in-single
> > -declaration-no-420-pack.vert
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-qualifier-in-single-declaration-420
> > -pack.vert
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-qualifier-in-single-declaration-no
> > -420-pack.vert
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-stream-in-single-declaration-420-pack.geom
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-stream-in-single-declaration-no-420-pack.geom
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-stream-in-single-global-declaration-420
> > -pack.geom
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-stream-in-single-global-declaration-no-420
> > -pack.geom
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-stream-multiple-qualifier-in-single
> > -declaration-420-pack.geom
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-stream-multiple-qualifier-in-single
> > -declaration-no-420-pack.geom
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-stream-multiple-qualifier-in-single-global
> > -declaration-420-pack.geom
> >  create mode 100644
> > tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-stream-multiple-qualifier-in-single-global
> > -declaration-no-420-pack.geom
> > 
> > diff --git
> > a/tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-multiple-qualifier-in-single
> > -declaration-420-pack.vert
> > b/tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-multiple-qualifier-in-single
> > -declaration-420-pack.vert
> > new file mode 100644
> > index 0000000..9551476
> > --- /dev/null
> > +++ b/tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-multiple-qualifier-in-single
> > -declaration-420-pack.vert
> > @@ -0,0 +1,22 @@
> > +// [config]
> > +// expect_result: pass
> > +// glsl_version: 1.40
> > +// require_extensions: GL_ARB_separate_shader_objects
> > GL_ARB_shading_language_420pack
> > +// check_link: false
> > +// [end config]
> > +//
> > +// From the ARB_shading_language_420pack spec:
> > +//
> > +//   "More than one layout qualifier may appear in a single
> > declaration. If
> > +//   the same layout-qualifier-name occurs in multiple layout
> > qualifiers for
> > +//   the same declaration, the last one overrides the former
> > ones."
> > +
> > +#version 140
> > +#extension GL_ARB_separate_shader_objects : enable
> > +#extension GL_ARB_shading_language_420pack: enable
> > +
> > +layout(location=2) layout(location=1) out vec3 var;
> > +
> > +void main()
> > +{
> > +}
> > diff --git
> > a/tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-multiple-qualifier-in-single
> > -declaration-no-420-pack.vert
> > b/tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-multiple-qualifier-in-single
> > -declaration-no-420-pack.vert
> > new file mode 100644
> > index 0000000..e5cbeb4
> > --- /dev/null
> > +++ b/tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-multiple-qualifier-in-single
> > -declaration-no-420-pack.vert
> > @@ -0,0 +1,21 @@
> > +// [config]
> > +// expect_result: fail
> > +// glsl_version: 1.40
> > +// require_extensions: GL_ARB_separate_shader_objects
> > +// check_link: false
> > +// [end config]
> > +//
> > +// From the ARB_shading_language_420pack spec:
> > +//
> > +//   "More than one layout qualifier may appear in a single
> > declaration. If
> > +//   the same layout-qualifier-name occurs in multiple layout
> > qualifiers for
> > +//   the same declaration, the last one overrides the former
> > ones."
> > +
> > +#version 140
> > +#extension GL_ARB_separate_shader_objects : enable
> > +
> > +layout(location=2) layout(location=1) out vec3 var;
> > +
> > +void main()
> > +{
> > +}
> > diff --git
> > a/tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-qualifier-in-single-declaration-420
> > -pack.vert
> > b/tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-qualifier-in-single-declaration-420
> > -pack.vert
> > new file mode 100644
> > index 0000000..cff5d51
> > --- /dev/null
> > +++ b/tests/spec/arb_shading_language_420pack/compiler/layout
> > -qualifiers/multiple-location-qualifier-in-single-declaration-420
> > -pack.vert
> > @@ -0,0 +1,33 @@
> > +// [config]
> > +// expect_result: pass
> > +// glsl_version: 1.40
> > +// require_extensions: GL_ARB_separate_shader_objects
> > GL_ARB_shading_language_420pack
> > +// check_link: false
> > +// [end config]
> > +//
> > +// From the ARB_shading_language_420pack spec:
> > +//
> > +//   "More than one layout qualifier may appear in a single
> > declaration. If
> > +//   the same layout-qualifier-name occurs in multiple layout
> > qualifiers for
> > +//   the same declaration, the last one overrides the former
> > ones."
> > +//
> > +// While the spec is taking about multiple layout qualifiers we
> > interpert that
> 
> typo: interpret
> 
> Occurs elsewhere as well.