Re: [Piglit] [PATCH v2 3/3] cl: Replace handwritten vload tests with a generator

[Dylan Baker]

Quoting Jan Vesely (2017-08-23 12:38:53)
> Hi Dylan,
> 
> do you mind taking a look at the python parts?
> 
> thanks,
> Jan

Sure, I have a few comments, but mostly this looks okay, my comments are mostly
style nits anyway.

Dylan

> On Wed, 2017-08-16 at 20:39 -0400, Jan Vesely wrote:
> > v2: simplify
> > mark local storage volatile
> > Passes on beignet(IVB), and intel CPU
> > 
> > Signed-off-by: Jan Vesely 
> > ---
> > clover on carrizo passes as well, apart from vload_half tests, because
> > the function is missing in libclc
> > 
> > +

[snip]

> > +from __future__ import print_function, division, absolute_import
> > +import os
> > +import textwrap
> > +import random

please sort the os, textwrap, and radom imports

> > +
> > +from six.moves import range
> > +
> > +from modules import utils
> > +
> > +TYPES = ['char', 'uchar', 'short', 'ushort', 'int', 'uint', 'long', 
> > 'ulong', 'half', 'float', 'double']
> > +VEC_SIZES = ['2', '4', '8', '16']
> > +
> > +dirName = os.path.join("cl", "vload")

module level constants should be all caps with underscores like TYPES and
VEC_SIZES

> > +
> > +
> > +def gen_array(size):
> > +random.seed(size)
> > +return [str(random.randint(0, 255)) for i in range(size)]
> > +
> > +
> > +def ext_req(type_name):
> > +if type_name[:6] == "double":
> > +return "require_device_extensions: cl_khr_fp64"
> > +if type_name[:6] == "half":

Should this be [:4]?

> > +return "require_device_extensions: cl_khr_fp16"
> > +return ""
> > +
> > +def begin_test(suffix, type_name, mem_type, vec_sizes, addr_space):
> > +fileName = os.path.join(dirName, 'vload'+ suffix + '-' + type_name + 
> > '-' + addr_space + '.cl')

I think that using str.format here is much more readable:
fileName = os.path.join(dirName, "vload{}-{}-{}.cl".format(suffix, type_name, 
addr_space))

Also, could you use file_name instead of fileName, in keeping with our python
style?

> > +print(fileName)
> > +f = open(fileName, 'w')
> > +f.write(textwrap.dedent(("""

You can add a \ to the end of """ to avoid an extra newline at the top of the
file, if you like

> > +/*!
> > +[config]
> > +name: Vector load{suffix} {addr_space} {type_name}2,4,8,16
> > +clc_version_min: 10
> > +
> > +dimensions: 1
> > +global_size: 1 0 0
> > +""" + ext_req(type_name))
> > +.format(type_name=type_name, addr_space=addr_space, suffix=suffix)))
> > +for s in vec_sizes:
> > +size = int(s) if s != '' else 1
> > +data_array = gen_array(size)
> > +ty_name = type_name + s
> > +f.write(textwrap.dedent("""
> > +[test]
> > +name: vector load{suffix} {addr_space} {type_name}
> > +kernel_name: vload{suffix}{n}_{addr_space}
> > +arg_in:  0 buffer {mem_type}[{size}] 0 {gen_array}
> > +arg_out: 1 buffer {type_name}[2] {first_array} {gen_array}
> > +
> > +[test]
> > +name: vector load{suffix} {addr_space} offset {type_name}
> > +kernel_name: vload{suffix}{n}_{addr_space}_offset
> > +arg_in:  0 buffer {mem_type}[{offset_size}] {zeros}{gen_array}
> > +arg_out: 1 buffer {type_name}[2] {first_array} {gen_array}
> > +""".format(type_name=ty_name, mem_type=mem_type, size=size + 1,
> > +   zeros=("0 " * (size + 1)), offset_size=size*2 + 1, n=s,

Spaces around the * operator please (offset_size=size * 2 + 1,)

> > +   gen_array=' '.join(data_array), suffix=suffix,
> > +   addr_space=addr_space,
> > +   first_array="0 " + ' '.join(data_array[:-1]
> > +
> > +f.write(textwrap.dedent("""
> > +!*/
> > +"""))
> > +if type_name == "double":
> > +f.write(textwrap.dedent("""
> > +#pragma OPENCL EXTENSION cl_khr_fp64: enable
> > +"""))
> > +if type_name == "half":
> > +f.write(textwrap.dedent("""
> > +#pragma OPENCL EXTENSION cl_khr_fp16: enable
> > +"""))
> > +return f

Two new lines between top level functions and classes please.

> > +
> > +def gen_test_constant_global(suffix, t, mem_type, vec_sizes, addr_space):
> > +f = begin_test(suffix, t, mem_type, vec_sizes, addr_space)
> > +for s in vec_sizes:
> > +type_name = t + s
> > +f.write(textwrap.dedent("""
> > +kernel void vload{suffix}{n}_{addr_space}({addr_space} {mem_type} 
> > *in,
> > + global {type_name} *out) {{
> > +out[0] = vload{suffix}{n}(0, in);
> > +out[1] = vload{suffix}{n}(0, in + 1);
> > +}}
> > +
> > +kernel void vload{suffix}{n}_{addr_space}_offset({addr_space} 
> > {mem_type} *in,
> > +global {type_name} *out) {{
> > +out[0] = vload{suffix}{n}(1, in);
> > +out[1] = vload{suffix}{n}(1, in + 1);
> > +}}
> > +""".format(type_name=type_name, 

Re: [Piglit] [PATCH v2 3/3] cl: Replace handwritten vload tests with a generator

[Jan Vesely]

Hi Dylan,

do you mind taking a look at the python parts?

thanks,
Jan

On Wed, 2017-08-16 at 20:39 -0400, Jan Vesely wrote:
> v2: simplify
> mark local storage volatile
> Passes on beignet(IVB), and intel CPU
> 
> Signed-off-by: Jan Vesely 
> ---
> clover on carrizo passes as well, apart from vload_half tests, because
> the function is missing in libclc
> 
>  generated_tests/CMakeLists.txt |   4 +
>  generated_tests/gen_cl_vload_tests.py  | 212 
> +
>  tests/cl.py|   2 +
>  tests/cl/program/execute/vload-constant-int.cl |  64 
>  tests/cl/program/execute/vload-int.cl  | 175 
>  tests/cl/program/execute/vload-local-int.cl| 105 
>  tests/cl/program/execute/vload-private-int.cl  | 105 
>  7 files changed, 218 insertions(+), 449 deletions(-)
>  create mode 100644 generated_tests/gen_cl_vload_tests.py
>  delete mode 100644 tests/cl/program/execute/vload-constant-int.cl
>  delete mode 100644 tests/cl/program/execute/vload-int.cl
>  delete mode 100644 tests/cl/program/execute/vload-local-int.cl
>  delete mode 100644 tests/cl/program/execute/vload-private-int.cl
> 
> diff --git a/generated_tests/CMakeLists.txt b/generated_tests/CMakeLists.txt
> index 44572bdf6..fe82ccfa4 100644
> --- a/generated_tests/CMakeLists.txt
> +++ b/generated_tests/CMakeLists.txt
> @@ -214,6 +214,9 @@ piglit_make_generated_tests(
>   cl_vstore_tests.list
>   gen_cl_vstore_tests.py)
>  piglit_make_generated_tests(
> + cl_vload_tests.list
> + gen_cl_vload_tests.py)
> +piglit_make_generated_tests(
>   builtin_cl_math_tests.list
>   gen_cl_math_builtins.py)
>  piglit_make_generated_tests(
> @@ -271,6 +274,7 @@ add_custom_target(gen-cl-tests
>   builtin_cl_common_tests.list
>   cl_store_tests.list
>   cl_vstore_tests.list
> + cl_vload_tests.list
>  )
>  
>  # Add a "gen-tests" target that can be used to generate all the
> diff --git a/generated_tests/gen_cl_vload_tests.py 
> b/generated_tests/gen_cl_vload_tests.py
> new file mode 100644
> index 0..7ebb20fa7
> --- /dev/null
> +++ b/generated_tests/gen_cl_vload_tests.py
> @@ -0,0 +1,212 @@
> +# Copyright 2016 Advanced Micro Devices, Inc.
> +#
> +# 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.
> +
> +from __future__ import print_function, division, absolute_import
> +import os
> +import textwrap
> +import random
> +
> +from six.moves import range
> +
> +from modules import utils
> +
> +TYPES = ['char', 'uchar', 'short', 'ushort', 'int', 'uint', 'long', 'ulong', 
> 'half', 'float', 'double']
> +VEC_SIZES = ['2', '4', '8', '16']
> +
> +dirName = os.path.join("cl", "vload")
> +
> +
> +def gen_array(size):
> +random.seed(size)
> +return [str(random.randint(0, 255)) for i in range(size)]
> +
> +
> +def ext_req(type_name):
> +if type_name[:6] == "double":
> +return "require_device_extensions: cl_khr_fp64"
> +if type_name[:6] == "half":
> +return "require_device_extensions: cl_khr_fp16"
> +return ""
> +
> +def begin_test(suffix, type_name, mem_type, vec_sizes, addr_space):
> +fileName = os.path.join(dirName, 'vload'+ suffix + '-' + type_name + '-' 
> + addr_space + '.cl')
> +print(fileName)
> +f = open(fileName, 'w')
> +f.write(textwrap.dedent(("""
> +/*!
> +[config]
> +name: Vector load{suffix} {addr_space} {type_name}2,4,8,16
> +clc_version_min: 10
> +
> +dimensions: 1
> +global_size: 1 0 0
> +""" + ext_req(type_name))
> +.format(type_name=type_name, addr_space=addr_space, suffix=suffix)))
> +for s in vec_sizes:
> +size = int(s) if s != '' else 1
> +data_array = gen_array(size)
> +ty_name = type_name + s
> +

Re: [Piglit] [PATCH] draw-pixels: fix KHR_no_error logic

[Samuel Pitoiset]

On 08/23/2017 06:11 AM, Timothy Arceri wrote:

---

  This was my fault. The flaw was in my suggestion from the code
  review.


You probably need to use PIGLIT_HAS_ERRORS as well.



  tests/general/draw-pixels.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/general/draw-pixels.c b/tests/general/draw-pixels.c
index 40b4c0b0f..333bb7f86 100644
--- a/tests/general/draw-pixels.c
+++ b/tests/general/draw-pixels.c
@@ -730,22 +730,24 @@ piglit_display(void)
  
  	glPixelStorei(GL_UNPACK_ALIGNMENT, 1);
  
  	for (i = 0; i < ARRAY_SIZE(data_types); i++) {

for (k = 0; k < ARRAY_SIZE(pixel_ops); k++) {
for (j = 0; j < ARRAY_SIZE(pixel_formats); j++) {
  
  format = pixel_formats[j];

type = data_types[i];
  
-if (!piglit_khr_no_error &&

-   is_format_type_mismatch(format, type)) {
+   if (is_format_type_mismatch(format, type)) {
+   if (piglit_khr_no_error)
+   continue;
+
glDrawPixels(piglit_width, 
piglit_height,
 format, type, pixels);
/* Here GL_INVALID_OPERATION is an
 * expected GL error
 */
pass = piglit_check_gl_error(
   GL_INVALID_OPERATION)
   && pass;
continue;
}


___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit