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

[Nicolai Hähnle]

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?

[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.

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 :)

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]