[Piglit] [PATCH v2 2/2] add ARB_compute_variable_group_size tests

[Samuel Pitoiset]

On 09/28/2016 12:44 PM, Nicolai Hähnle wrote:
> On 10.09.2016 17:19, Samuel Pitoiset wrote:
>> v1: - update formatting spec quotations (Nicolai)
>>     - use shader_runner for the basic test (Ian)
>>     - add a new test which checks various local sizes (Nicolai)
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>  tests/all.py                                       |   8 +
>>  tests/spec/CMakeLists.txt                          |   1 +
>>  .../CMakeLists.gl.txt                              |  16 ++
>>  .../arb_compute_variable_group_size/CMakeLists.txt |   1 +
>>  .../compiler/do_nothing.comp                       |  14 +
>>  .../compiler/gl_LocalGroupSizeARB_illegal_use.comp |  28 ++
>>  .../compiler/gl_LocalGroupSizeARB_layout.comp      |  27 ++
>>  .../mixed_fixed_variable_local_work_size.comp      |  23 ++
>>  .../spec/arb_compute_variable_group_size/errors.c  | 249
>> ++++++++++++++++++
>>  .../execution/basic-local-size.shader_test         |  31 +++
>>  ...ixed_fixed_variable_local_work_size.shader_test |  36 +++
>>  .../linker/no_local_size_specified.shader_test     |  32 +++
>>  .../arb_compute_variable_group_size/local-size.c   | 283
>> +++++++++++++++++++++
>>  .../spec/arb_compute_variable_group_size/minmax.c  |  65 +++++
>>  14 files changed, 814 insertions(+)
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/CMakeLists.gl.txt
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/CMakeLists.txt
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/compiler/do_nothing.comp
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/compiler/gl_LocalGroupSizeARB_illegal_use.comp
>>
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/compiler/gl_LocalGroupSizeARB_layout.comp
>>
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/compiler/mixed_fixed_variable_local_work_size.comp
>>
>>  create mode 100644 tests/spec/arb_compute_variable_group_size/errors.c
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/execution/basic-local-size.shader_test
>>
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/linker/mixed_fixed_variable_local_work_size.shader_test
>>
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/linker/no_local_size_specified.shader_test
>>
>>  create mode 100644
>> tests/spec/arb_compute_variable_group_size/local-size.c
>>  create mode 100644 tests/spec/arb_compute_variable_group_size/minmax.c
>>
> [snip]
>> diff --git
>> a/tests/spec/arb_compute_variable_group_size/compiler/mixed_fixed_variable_local_work_size.comp
>> b/tests/spec/arb_compute_variable_group_size/compiler/mixed_fixed_variable_local_work_size.comp
>>
>> new file mode 100644
>> index 0000000..7bcfb4e
>> --- /dev/null
>> +++
>> b/tests/spec/arb_compute_variable_group_size/compiler/mixed_fixed_variable_local_work_size.comp
>>
>> @@ -0,0 +1,23 @@
>> +// [config]
>> +// expect_result: fail
>> +// glsl_version: 3.30
>> +// require_extensions: GL_ARB_compute_variable_group_size
>> +// [end config]
>> +//
>> +// From the ARB_compute_variable_group_size spec:
>> +//
>> +//     If a compute shader including a *local_size_variable*
>> qualifier also
>> +//     declares a fixed local group size using the *local_size_x*,
>> +//     *local_size_y*, or *local_size_z* qualifiers, a compile-time
>> error
>> +//     results.
>> +
>> +#version 330
>> +#extension GL_ARB_compute_variable_group_size: enable
>> +#extension GL_ARB_compute_shader: enable
>> +
>> +layout(local_size_x = 2) in;
>> +layout(local_size_variable) in;
>
> Maybe add a test where the order of these is reversed?

Not sure if it's really useful, but I added one.

>
> [snip]
>> diff --git a/tests/spec/arb_compute_variable_group_size/local-size.c
>> b/tests/spec/arb_compute_variable_group_size/local-size.c
>> new file mode 100644
>> index 0000000..301a920
>> --- /dev/null
>> +++ b/tests/spec/arb_compute_variable_group_size/local-size.c
> [snip]
>> +static enum piglit_result
>> +test_all_sizes()
>> +{
>> +    enum piglit_result result = PIGLIT_PASS;
>> +    uint32_t xi, yi, zi;
>> +    uint32_t x, y, z;
>> +
>> +
>> +    for (zi = 0; zi < ARRAY_SIZE(sizes); zi++) {
>> +        z = sizes[zi];
>> +        if (z > max_local_z)
>> +            break;
>> +        for (yi = 0; yi < ARRAY_SIZE(sizes); yi++) {
>> +            y = sizes[yi];
>> +            if ((y * z) > max_local_y)
>> +                break;
>
> I think this test should be
>
>    if (y > max_local_y || y * z > max_variable_threads)
>
> with max_variable_threads determined in piglit_init.
>
>
>> +            for (xi = 0; xi < ARRAY_SIZE(sizes); xi++) {
>> +                x = sizes[xi];
>> +                if ((x * y * z) > max_local_x)
>> +                    break;
>
> ... and analogously here.

Yeah, fixed, this enforce the test.
>
> With those two minor comments addressed, the series is
>
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> No need to send a v3 as far as I'm concerned :)

Thanks!

>
> Cheers,
> Nicolai
>
>> +                result = test_size(x, y, z);
>> +                if (result != PIGLIT_PASS)
>> +                    return result;
>> +            }
>> +        }
>> +    }
>> +
>> +    return result;
>> +}
>> +
>> +enum piglit_result
>> +piglit_display(void)
>> +{
>> +    return PIGLIT_FAIL;
>> +}
>> +
>> +void
>> +piglit_init(int argc, char **argv)
>> +{
>> +    enum piglit_result result;
>> +
>> +    piglit_require_extension("GL_ARB_compute_variable_group_size");
>> +    piglit_require_extension("GL_ARB_shader_atomic_counters");
>> +
>> +    glGenBuffers(1, &atomics_bo);
>> +    if (!piglit_check_gl_error(GL_NO_ERROR))
>> +        piglit_report_result(PIGLIT_FAIL);
>> +
>> +    glGetIntegeri_v(GL_MAX_COMPUTE_VARIABLE_GROUP_SIZE_ARB,
>> +            0, &max_local_x);
>> +    glGetIntegeri_v(GL_MAX_COMPUTE_VARIABLE_GROUP_SIZE_ARB,
>> +            1, &max_local_y);
>> +    glGetIntegeri_v(GL_MAX_COMPUTE_VARIABLE_GROUP_SIZE_ARB,
>> +            2, &max_local_z);
>> +
>> +    result = test_all_sizes();
>> +
>> +    glDeleteBuffers(1, &atomics_bo);
>> +
>> +    piglit_report_result(result);
>> +}
> [snip]