[Piglit] [PATCH 1/7] cl-program-tester: Use more appropriate member names for struct test_arg
Tom Stellard
tom at stellard.net
Wed Oct 2 09:50:46 PDT 2013
On Tue, Oct 01, 2013 at 05:03:16PM -0500, Aaron Watry wrote:
> The main downside to this plan that I see is that combining the tests
> for all data types into the same program run leads to something like
> the following:
>
> Using the current vload-int.cl as an example:
> Currently my pitcairn fails to run the test at all due to issues with
> the vload3, vload8, and vload16 tests. If I remove the int3/8/16
> tests, everything passes. Currently, we abort the entire test program
> if one of the data sizes triggers an LLVM-based crash. So instead of
> seeing int2/int4 working and the rest failing, I just see a crash when
> it first tries to compile the int3 kernel (which happens when it tries
> to execute the first int2 test).
>
> If we split each data type and vector size into their own files or
> test processes, we lose a bit/lot on piglit run speeds, but we would
> get full results from the test run. I.e. if int3 fails but everything
> else is valid, I'd love to just see crashes for the int3 tests, but
> passes for all of the other tests, instead of a truncated test run.
>
I agree this is a major problem. However, it's really a problem with our
driver, and I'm not sure we want to work around it in piglit. Basically,
the driver crashes in the compiler way too much. It really should never
crash and instead return the appropriate error on failure. There are
two ways to fix this: 1. Update the LLVM infrastructure to return
an error rather than crashing or 2. Fix the driver so it doesn't crash.
It would be great if we could solve it using solution #1, but I'm not
sure how much work that would be.
I also would like to see test results for every test, but I think it's
more important to keep the piglit run time as low as possible. It
already takes too long in my opinion and we don't really have that many
tests (relative to OpenGL anyway). However, I haven't actually profiled
piglit, so I can't say for certain that test parsing overhead is the
reason it is so slow. So, I think I should probably do that first
before pushing forward with more gentype conversions.
Even if we don't end up using it everywhere, I still think it is a
useful feature in some cases.
-Tom
> It doesn't sound like you're headed in that direction, but it's also
> the same result that we currently have, so it's not really a
> regression in functionality either. I do like that we'll get
> additional test coverage and simplified test writing out of these
> changes. If we wanted to split each gentype into its own separate
> process, that can probably be done at a future time.
>
> --Aaron
>
>
> On Tue, Oct 1, 2013 at 9:24 AM, Tom Stellard <tom at stellard.net> wrote:
> > On Mon, Sep 30, 2013 at 05:09:46PM -0500, Aaron Watry wrote:
> >> I hope to have time to dig into the actual changes themselves in more
> >> detail (my first skim through looked fairly good besides my comment on
> >> patch 6). If I don't get time to do a full review of the changes, I
> >> just wanted to mention that I tested this on my CEDAR without any
> >> regressions to existing tests... although the new kernel_args test
> >> doesn't pass (but maybe that's expected).
> >>
> >
> > Thanks for testing. The kernel_args test fails for me too. I'm still
> > working on fix for it.
> >
> > My long term plan with the gentype feature is to make all the python
> > generated tests use gentype. This will give us more test cases for
> > the gentype feature and will hopefully reduce piglit run times by reducing
> > the number of test files.
> >
> > -Tom
> >
> >>
> >> On Mon, Sep 30, 2013 at 9:47 AM, Tom Stellard <tom at stellard.net> wrote:
> >> > From: Tom Stellard <thomas.stellard at amd.com>
> >> >
> >> > cl_size has been renamed to vec_elements and cl_mem_size has been
> >> > renamed to vec_mem_elements. This use of size is confusing, because
> >> > size generally means number of bytes when used to describe a type, and
> >> > in this case the value being stored was the number of elements in the
> >> > vector.
> >> > ---
> >> > tests/cl/program/program-tester.c | 48 +++++++++++++++++++--------------------
> >> > 1 file changed, 24 insertions(+), 24 deletions(-)
> >> >
> >> > diff --git a/tests/cl/program/program-tester.c b/tests/cl/program/program-tester.c
> >> > index be6bc20..1524db0 100644
> >> > --- a/tests/cl/program/program-tester.c
> >> > +++ b/tests/cl/program/program-tester.c
> >> > @@ -216,8 +216,8 @@ struct test_arg {
> >> > enum test_arg_type type;
> >> >
> >> > enum cl_type cl_type;
> >> > - size_t cl_size; // 1 for int, 3 for int3
> >> > - size_t cl_mem_size; // 1 for int, 4 for int3
> >> > + size_t vec_elements; // 1 for int, 3 for int3
> >> > + size_t vec_mem_elements; // 1 for int, 4 for int3
> >> > size_t length; // for buffers
> >> >
> >> > /* kernel arg data */
> >> > @@ -237,8 +237,8 @@ struct test_arg create_test_arg()
> >> > .type = TEST_ARG_VALUE,
> >> >
> >> > .cl_type = TYPE_CHAR,
> >> > - .cl_size = 1,
> >> > - .cl_mem_size = 1,
> >> > + .vec_elements = 1,
> >> > + .vec_mem_elements = 1,
> >> > .length = 0,
> >> >
> >> > .index = 0,
> >> > @@ -868,9 +868,9 @@ get_test_arg_value(struct test_arg* test_arg, const char* value, size_t length)
> >> > case enum_type: \
> >> > get_func(value, &array, length); \
> >> > for(i = 0; i < test_arg->length; i++) { \
> >> > - for(c = 0; c < test_arg->cl_size; c++) { \
> >> > - ra = i*test_arg->cl_size + c; \
> >> > - rb = i*test_arg->cl_mem_size + c; \
> >> > + for(c = 0; c < test_arg->vec_elements; c++) { \
> >> > + ra = i*test_arg->vec_elements + c; \
> >> > + rb = i*test_arg->vec_mem_elements + c; \
> >> > ((cl_type*)test_arg->value)[rb] = array[ra%length]; \
> >> > } \
> >> > } \
> >> > @@ -963,23 +963,23 @@ get_test_arg(const char* src, struct test* test, bool arg_in)
> >> > test_arg.index = get_int(index_str);
> >> > free(index_str);
> >> >
> >> > - /* Set type, cl_size, cl_mem_size and size (partially for buffers) */
> >> > + /* Set type, vec_elements, vec_mem_elements and size (partially for buffers) */
> >> > regex_get_match_str(&type, src, pmatch, 2);
> >> > if(regex_match(type, "[[:digit:]]+")) {
> >> > char* type_size_str;
> >> > regex_get_str(&type_size_str, type, "[[:digit:]]+", 0, REG_NEWLINE);
> >> > - test_arg.cl_size = get_int(type_size_str);
> >> > - test_arg.cl_mem_size = test_arg.cl_size != 3 ? test_arg.cl_size : 4; // test if we have type3
> >> > + test_arg.vec_elements = get_int(type_size_str);
> >> > + test_arg.vec_mem_elements = test_arg.vec_elements != 3 ? test_arg.vec_elements : 4; // test if we have type3
> >> > free(type_size_str);
> >> > } else {
> >> > - test_arg.cl_size = 1;
> >> > - test_arg.cl_mem_size = 1;
> >> > + test_arg.vec_elements = 1;
> >> > + test_arg.vec_mem_elements = 1;
> >> > }
> >> >
> >> > #define IF(regex_type, enum_type, main_type) \
> >> > if(regex_match(type, REGEX_FULL_MATCH(regex_type))) { \
> >> > test_arg.cl_type = enum_type; \
> >> > - test_arg.size = sizeof(main_type) * test_arg.cl_mem_size; \
> >> > + test_arg.size = sizeof(main_type) * test_arg.vec_mem_elements; \
> >> > }
> >> > #define ELSEIF(regex_type, enum_type, main_type) \
> >> > else IF(regex_type, enum_type, main_type)
> >> > @@ -1020,7 +1020,7 @@ get_test_arg(const char* src, struct test* test, bool arg_in)
> >> > if(regex_match(value, REGEX_FULL_MATCH(REGEX_NULL))) {
> >> > test_arg.value = NULL;
> >> > } else {
> >> > - get_test_arg_value(&test_arg, value, test_arg.cl_size);
> >> > + get_test_arg_value(&test_arg, value, test_arg.vec_elements);
> >> > }
> >> > free(value);
> >> > } else if(regex_match(src, REGEX_FULL_MATCH(REGEX_ARG_BUFFER))) { // buffer
> >> > @@ -1084,7 +1084,7 @@ get_test_arg(const char* src, struct test* test, bool arg_in)
> >> > } else if(regex_match(value, REGEX_ARRAY)) {
> >> > get_test_arg_value(&test_arg,
> >> > value,
> >> > - test_arg.length * test_arg.cl_size);
> >> > + test_arg.length * test_arg.vec_elements);
> >> > }
> >> > }
> >> > free(value);
> >> > @@ -1623,12 +1623,12 @@ check_test_arg_value(struct test_arg test_arg,
> >> > #define CASEI(enum_type, type, cl_type) \
> >> > case enum_type: \
> >> > for(i = 0; i < test_arg.length; i++) { \
> >> > - for(c = 0; c < test_arg.cl_size; c++) { \
> >> > - rb = i*test_arg.cl_mem_size + c; \
> >> > + for(c = 0; c < test_arg.vec_elements; c++) { \
> >> > + rb = i*test_arg.vec_mem_elements + c; \
> >> > if(!piglit_cl_probe_integer(((cl_type*)value)[rb], \
> >> > ((cl_type*)test_arg.value)[rb], \
> >> > test_arg.toli)) { \
> >> > - ra = i*test_arg.cl_size + c; \
> >> > + ra = i*test_arg.vec_elements + c; \
> >> > printf("Error at %s[%zu]\n", type, ra); \
> >> > return false; \
> >> > } \
> >> > @@ -1638,12 +1638,12 @@ check_test_arg_value(struct test_arg test_arg,
> >> > #define CASEU(enum_type, type, cl_type) \
> >> > case enum_type: \
> >> > for(i = 0; i < test_arg.length; i++) { \
> >> > - for(c = 0; c < test_arg.cl_size; c++) { \
> >> > - rb = i*test_arg.cl_mem_size + c; \
> >> > + for(c = 0; c < test_arg.vec_elements; c++) { \
> >> > + rb = i*test_arg.vec_mem_elements + c; \
> >> > if(!piglit_cl_probe_uinteger(((cl_type*)value)[rb], \
> >> > ((cl_type*)test_arg.value)[rb], \
> >> > test_arg.tolu)) { \
> >> > - ra = i*test_arg.cl_size + c; \
> >> > + ra = i*test_arg.vec_elements + c; \
> >> > printf("Error at %s[%zu]\n", type, ra); \
> >> > return false; \
> >> > } \
> >> > @@ -1653,12 +1653,12 @@ check_test_arg_value(struct test_arg test_arg,
> >> > #define CASEF(enum_type, type, cl_type) \
> >> > case enum_type: \
> >> > for(i = 0; i < test_arg.length; i++) { \
> >> > - for(c = 0; c < test_arg.cl_size; c++) { \
> >> > - rb = i*test_arg.cl_mem_size + c; \
> >> > + for(c = 0; c < test_arg.vec_elements; c++) { \
> >> > + rb = i*test_arg.vec_mem_elements + c; \
> >> > if(!piglit_cl_probe_floating(((cl_type*)value)[rb], \
> >> > ((cl_type*)test_arg.value)[rb], \
> >> > test_arg.tolf)) { \
> >> > - ra = i*test_arg.cl_size + c; \
> >> > + ra = i*test_arg.vec_elements + c; \
> >> > printf("Error at %s[%zu]\n", type, ra); \
> >> > return false; \
> >> > } \
> >> > --
> >> > 1.7.11.4
> >> >
> >> > _______________________________________________
> >> > Piglit mailing list
> >> > Piglit at lists.freedesktop.org
> >> > http://lists.freedesktop.org/mailman/listinfo/piglit
More information about the Piglit
mailing list