arb_shader_image_load_store: test duplicate format qualifier more thoroughly

Submitted by Timothy Arceri on Jan. 20, 2016, 4:39 a.m.

Details

Message ID 1453264759-19439-1-git-send-email-timothy.arceri@collabora.com
State New
Headers show
Series "arb_shader_image_load_store: test duplicate format qualifier more thoroughly" ( rev: 2 ) in Piglit

Not browsing as part of any series.

Commit Message

Timothy Arceri Jan. 20, 2016, 4:39 a.m.
From the ARB_shader_image_load_store spec:

   "Only one format qualifier may be specified for any image variable
    declaration."

ARB_enhanced_layouts

  Allows duplicates within a single layout qualifier e.g.

  layout(location = 0, location = 1) out vec4 a;

ARB_shading_language_420pack

  Allows multiple layout qualifiers e.g.

  layout(location = 0) layout(location = 2) out vec4 b;

However layout(rgba32f, rgba32f) uniform image2D img;
and layout(rgba32f) layout(rgba32f) uniform image2D img;

Should still fail when these extensions are enabled according to the
spec GLSL 4.5 spec.

Cc: Francisco Jerez <currojerez@riseup.net>
---
 .../gen_shader_image_load_store_tests.py           | 39 ++++++++++++++++++----
 1 file changed, 32 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/generated_tests/gen_shader_image_load_store_tests.py b/generated_tests/gen_shader_image_load_store_tests.py
index 8d6be9c..d52070c 100644
--- a/generated_tests/gen_shader_image_load_store_tests.py
+++ b/generated_tests/gen_shader_image_load_store_tests.py
@@ -29,24 +29,28 @@  from textwrap import dedent
 from modules import utils
 
 
-def gen_header(status):
+def gen_header(status, extra_extension=''):
     """
     Generate a GLSL program header.
 
     Generate header code for ARB_shader_image_load_store GLSL parser
     tests that are expected to give status as result.
     """
+    extra_extension_s = ''
+    if extra_extension != '':
+        extra_extension_s = "#extension {0}: require".format(extra_extension)
     return dedent("""\
         /*
          * [config]
          * expect_result: {0}
          * glsl_version: 1.50
-         * require_extensions: GL_ARB_shader_image_load_store
+         * require_extensions: GL_ARB_shader_image_load_store {1}
          * [end config]
          */
         #version 150
         #extension GL_ARB_shader_image_load_store: require
-    """.format(status))
+        {2}
+    """.format(status, extra_extension, extra_extension_s))
 
 
 def gen_vector_type(scalar, dim):
@@ -458,20 +462,41 @@  gen('declaration-format-qualifier', """\
      'prefix': ''}
 ]))
 
-gen('declaration-format-qualifier-duplicate', """\
-    ${header('fail')}
+gen('declaration-format-qualifier', """\
+    ${header('fail', extra_extension)}
+    ${description}
     /*
      * From the ARB_shader_image_load_store spec:
      *
      * "Only one format qualifier may be specified for any image variable
      *  declaration."
      */
-    layout(rgba32f) layout(rgba32f) uniform image2D img;
+    ${layout_qualifier} uniform image2D img;
 
     void main()
     {
     }
-""", shader_stages)
+""", product(shader_stages, [
+    {'name': 'duplicate',
+     'extra_extension': '',
+     'description': '',
+     'layout_qualifier': 'layout(rgba32f) layout(rgba32f)'},
+    {'name': 'duplicate-420pack',
+     'extra_extension': 'GL_ARB_shading_language_420pack',
+     'description': '/* Check duplicates still fail with '
+     'GL_ARB_shading_language_420pack \n * which allows multiple layout '
+     'qualifers\n */',
+     'layout_qualifier': 'layout(rgba32f) layout(rgba32f)'},
+    {'name': 'duplicate-within-layout',
+     'extra_extension': '',
+     'description': '',
+     'layout_qualifier': 'layout(rgba32f, rgba32f)'},
+    {'name': 'duplicate-within-layout-enhanced-layouts',
+     'extra_extension': 'GL_ARB_enhanced_layouts',
+     'description': '/* Check duplicates still fail with ARB_enhanced_layouts'
+     ' \n * which allows duplicates in a single layout\n */',
+     'layout_qualifier': 'layout(rgba32f, rgba32f)'}
+]))
 
 gen('declaration-format-qualifier-missing', """\
     ${header(status)}

Comments

Timothy Arceri <timothy.arceri@collabora.com> writes:

> From the ARB_shader_image_load_store spec:
>
>    "Only one format qualifier may be specified for any image variable
>     declaration."
>
> ARB_enhanced_layouts
>
>   Allows duplicates within a single layout qualifier e.g.
>
>   layout(location = 0, location = 1) out vec4 a;
>
> ARB_shading_language_420pack
>
>   Allows multiple layout qualifiers e.g.
>
>   layout(location = 0) layout(location = 2) out vec4 b;
>
> However layout(rgba32f, rgba32f) uniform image2D img;
> and layout(rgba32f) layout(rgba32f) uniform image2D img;
>
> Should still fail when these extensions are enabled according to the
> spec GLSL 4.5 spec.
>
> Cc: Francisco Jerez <currojerez@riseup.net>
> ---
>  .../gen_shader_image_load_store_tests.py           | 39 ++++++++++++++++++----
>  1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/generated_tests/gen_shader_image_load_store_tests.py b/generated_tests/gen_shader_image_load_store_tests.py
> index 8d6be9c..d52070c 100644
> --- a/generated_tests/gen_shader_image_load_store_tests.py
> +++ b/generated_tests/gen_shader_image_load_store_tests.py
> @@ -29,24 +29,28 @@ from textwrap import dedent
>  from modules import utils
>  
>  
> -def gen_header(status):
> +def gen_header(status, extra_extension=''):
>      """
>      Generate a GLSL program header.
>  
>      Generate header code for ARB_shader_image_load_store GLSL parser
>      tests that are expected to give status as result.
>      """
> +    extra_extension_s = ''
> +    if extra_extension != '':
> +        extra_extension_s = "#extension {0}: require".format(extra_extension)
>      return dedent("""\
>          /*
>           * [config]
>           * expect_result: {0}
>           * glsl_version: 1.50
> -         * require_extensions: GL_ARB_shader_image_load_store
> +         * require_extensions: GL_ARB_shader_image_load_store {1}
>           * [end config]
>           */
>          #version 150
>          #extension GL_ARB_shader_image_load_store: require
> -    """.format(status))
> +        {2}
> +    """.format(status, extra_extension, extra_extension_s))
>  
>  

You can probably achieve the same more generally and readably by doing
something like:

| def gen_header(status, extra_extensions=[]):
[...]
|     extensions = ['GL_ARB_shader_image_load_store'] + extra_extensions
|     extension_enables = ['#extension {0}: require'.format(x) for x in extensions]
|
|     return dedent("""\
|         /*
|          * [config]
|          * expect_result: {0}
|          * glsl_version: 1.50
|          * require_extensions: {1}
|          * [end config]
|          */
|          #version 150
|          {2}
|      """).format(status, ' '.join(extensions), '\n'.join(extension_enables))

(note that I've swapped the order of application of dedent() with
respect to format() because the join of extension enables is not going
to give you indented text as result)

>  def gen_vector_type(scalar, dim):
> @@ -458,20 +462,41 @@ gen('declaration-format-qualifier', """\
>       'prefix': ''}
>  ]))
>  
> -gen('declaration-format-qualifier-duplicate', """\
> -    ${header('fail')}
> +gen('declaration-format-qualifier', """\
> +    ${header('fail', extra_extension)}
> +    ${description}

I wouldn't bother to pass a different description for each generated
subtest, it makes it kind of difficult to read the rationale from the
generator source.  I'd simply mention in the comment below that the same
test is repeated with different extension enables which have an
influence on whether duplicate layout qualifiers are considered valid...

>      /*
>       * From the ARB_shader_image_load_store spec:
>       *
>       * "Only one format qualifier may be specified for any image variable
>       *  declaration."
>       */
> -    layout(rgba32f) layout(rgba32f) uniform image2D img;
> +    ${layout_qualifier} uniform image2D img;
>  
>      void main()
>      {
>      }
> -""", shader_stages)
> +""", product(shader_stages, [
> +    {'name': 'duplicate',
> +     'extra_extension': '',
> +     'description': '',
> +     'layout_qualifier': 'layout(rgba32f) layout(rgba32f)'},
> +    {'name': 'duplicate-420pack',
> +     'extra_extension': 'GL_ARB_shading_language_420pack',
> +     'description': '/* Check duplicates still fail with '
> +     'GL_ARB_shading_language_420pack \n * which allows multiple layout '
> +     'qualifers\n */',
> +     'layout_qualifier': 'layout(rgba32f) layout(rgba32f)'},
> +    {'name': 'duplicate-within-layout',
> +     'extra_extension': '',
> +     'description': '',
> +     'layout_qualifier': 'layout(rgba32f, rgba32f)'},
> +    {'name': 'duplicate-within-layout-enhanced-layouts',
> +     'extra_extension': 'GL_ARB_enhanced_layouts',
> +     'description': '/* Check duplicates still fail with ARB_enhanced_layouts'
> +     ' \n * which allows duplicates in a single layout\n */',
> +     'layout_qualifier': 'layout(rgba32f, rgba32f)'}
> +]))
>
You're basically computing part of the cartesian product by hand here,
we could be even lazier like:

| gen('declaration-format-qualifier-duplicate', """\
|     ${header('fail', [extra_extension] if extended else [])}
[...]
| """, product(shader_stages, [
|     {'name': 'single-layout-qualifier',
|      'layout_qualifier': 'layout(rgba32f, rgba32f)',
|      'extra_extension': 'GL_ARB_enhanced_layouts'},
|     {'name': 'multiple-layout-qualifiers',
|      'layout_qualifier': 'layout(rgba32f) layout(rgba32f)',
|      'extra_extension': 'GL_ARB_shading_language_420pack'}
| ], [
|     {'name': 'unextended', 'extended': False },
|     {'name': 'extended', 'extended': True }
| ]))


>  gen('declaration-format-qualifier-missing', """\
>      ${header(status)}
> -- 
> 2.4.3