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

Jan Vesely jan.vesely at rutgers.edu
Thu Aug 24 16:57:23 UTC 2017


On Wed, 2017-08-23 at 14:54 -0700, Dylan Baker wrote:
> 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 <jan.vesely at rutgers.edu>
> > > ---
> > > 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, mem_type=mem_type, n=s, suffix=suffix,
> > > +                   addr_space=addr_space)))
> > > +
> > > +    f.close()
> > > +
> > > +def gen_test_local_private(suffix, t, mem_type, vec_sizes, addr_space):
> > > +    f = begin_test(suffix, t, mem_type, vec_sizes, addr_space)
> > > +    for s in vec_sizes:
> > > +        size = int(s) if s != '' else 1
> > > +        type_name = t + s
> > > +        f.write(textwrap.dedent("""
> > > +        kernel void vload{suffix}{n}_{addr_space}(global {mem_type} *in,
> > > +                                     global {type_name} *out) {{
> > > +            volatile {addr_space} {mem_type} loc[{size}];
> > > +            for (int i = 0; i < {size}; ++i)
> > > +                loc[i] = in[i];
> > > +
> > > +            out[0] = vload{suffix}{n}(0, ({addr_space} {mem_type}*)loc);
> > > +            out[1] = vload{suffix}{n}(0, ({addr_space} {mem_type}*)loc + 1);
> > > +        }}
> > > +
> > > +        kernel void vload{suffix}{n}_{addr_space}_offset(global {mem_type} *in,
> > > +                                            global {type_name} *out) {{
> > > +            volatile {addr_space} {mem_type} loc[{offset_size}];
> > > +            for (int i = 0; i < {offset_size}; ++i)
> > > +                loc[i] = in[i];
> > > +
> > > +            out[0] = vload{suffix}{n}(1, ({addr_space} {mem_type}*)loc);
> > > +            out[1] = vload{suffix}{n}(1, ({addr_space} {mem_type}*)loc + 1);
> > > +        }}
> > > +        """.format(type_name=type_name, mem_type=mem_type, n=s, suffix=suffix,
> > > +                   offset_size=size*2 + 1, size=size + 1, addr_space=addr_space)))
> > > +
> > > +    f.close()
> > > +
> > > +# vload_half is special, becuase CLC won't allow us to use half type without
> > > +# cl_khr_fp16
> > > +def gen_test_local_private_half(suffix, t, vec_sizes, addr_space):
> > > +    f = begin_test(suffix, t, 'half', vec_sizes, addr_space)
> > > +    for s in vec_sizes:
> > > +        size = int(s) if s != '' else 1
> > > +        type_name = t + s
> > > +        f.write(textwrap.dedent("""
> > > +        kernel void vload{suffix}{n}_{addr_space}(global half *in,
> > > +                                     global {type_name} *out) {{
> > > +            volatile {addr_space} short loc[{size}];
> > > +            for (int i = 0; i < {size}; ++i)
> > > +                loc[i] = ((global short *)in)[i];
> > > +
> > > +            out[0] = vload{suffix}{n}(0, ({addr_space} half*)loc);
> > > +            out[1] = vload{suffix}{n}(0, ({addr_space} half*)loc + 1);
> > > +        }}
> > > +
> > > +        kernel void vload{suffix}{n}_{addr_space}_offset(global half *in,
> > > +                                            global {type_name} *out) {{
> > > +            volatile {addr_space} short loc[{offset_size}];
> > > +            for (int i = 0; i < {offset_size}; ++i)
> > > +                loc[i] = ((global short *)in)[i];
> > > +
> > > +            out[0] = vload{suffix}{n}(1, ({addr_space} half*)loc);
> > > +            out[1] = vload{suffix}{n}(1, ({addr_space} half*)loc + 1);
> > > +        }}
> > > +        """.format(type_name=type_name, n=s, suffix=suffix,
> > > +                   offset_size=size*2 + 1, size=size + 1, addr_space=addr_space)))
> > > +
> > > +def gen_test_local(suffix, t, mem_type, vec_sizes):
> > > +    if (mem_type == 'half'):
> 
> We don't use parens around if statements in python, unless needed to logically
> group conditions.
> 
> > > +        gen_test_local_private_half(suffix, t, vec_sizes, 'local')
> > > +    else:
> > > +        gen_test_local_private(suffix, t, mem_type, vec_sizes, 'local')
> > > +
> > > +def gen_test_private(suffix, t, mem_type, vec_sizes):
> > > +    if (mem_type == 'half'):
> > > +        gen_test_local_private_half(suffix, t, vec_sizes, 'private')
> > > +    else:
> > > +        gen_test_local_private(suffix, t, mem_type, vec_sizes, 'private')
> > > +
> > > +def gen_test_global(suffix, t, mem_type, vec_sizes):
> > > +    gen_test_constant_global(suffix, t, mem_type, vec_sizes, 'global')
> > > +
> > > +def gen_test_constant(suffix, t, mem_type, vec_sizes):
> > > +    gen_test_constant_global(suffix, t, mem_type, vec_sizes, 'constant')
> > > +
> > > +def main():
> > > +    utils.safe_makedirs(dirName)
> > > +    for t in TYPES:
> > > +        gen_test_constant('', t, t, VEC_SIZES);
> > > +        gen_test_global('', t, t, VEC_SIZES);
> > > +        gen_test_local('', t, t, VEC_SIZES);
> > > +        gen_test_private('', t, t, VEC_SIZES);
> > > +
> > > +    # There's no vload_half for double type
> > > +    gen_test_constant('_half', 'float',  'half', [''] + VEC_SIZES);
> > > +    gen_test_global('_half', 'float',  'half', [''] + VEC_SIZES);
> > > +    gen_test_local('_half', 'float',  'half', [''] + VEC_SIZES);
> > > +    gen_test_private('_half', 'float',  'half', [''] + VEC_SIZES);
> > > +
> > > +if __name__ == '__main__':
> > > +    main()
> > > diff --git a/tests/cl.py b/tests/cl.py
> > > index f06b3f638..ffaefb574 100644
> > > --- a/tests/cl.py
> > > +++ b/tests/cl.py
> > > @@ -143,3 +143,5 @@ add_program_test_dir(grouptools.join('program', 'execute', 'store'),
> > >                       os.path.join(GENERATED_TESTS_DIR, 'cl', 'store'))
> > >  add_program_test_dir(grouptools.join('program', 'execute', 'vstore'),
> > >                       os.path.join(GENERATED_TESTS_DIR, 'cl', 'vstore'))
> > > +add_program_test_dir(grouptools.join('program', 'execute', 'vload'),
> > > +                     os.path.join(GENERATED_TESTS_DIR, 'cl', 'vload'))
> 
> [snip]
> 
> Otherwise this looks fine to me. Since this is all style nits I'll trust you to
> fix them as you see fit, I don't need to see another version:
> Reviewed-by: Dylan Baker <dylan at pnwbakers.com>

thanks a lot.
I applied the same fixes to vstore generator as well, and pushed both.

regards,
Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20170824/bf3f32e9/attachment.sig>


More information about the Piglit mailing list