Message ID | 20220713204716.3114529-2-martin@martin.st |
---|---|
State | Accepted |
Commit | d69d12a5b9236b9d2f1fd247ea452f84cdd1aaf9 |
Headers | show |
Series | [FFmpeg-devel,1/2] x86: Don't hardcode the height to 8 in sad8_xy2_mmx | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
On Wed, 13 Jul 2022, Martin Storsjö wrote: > Previously, the checkasm test always passed h=8, so no other cases > were tested. > > Out of the me_cmp functions, in practice, some functions are hardcoded > to always assume a 8x8 block (ignoring the h parameter), while others > do use the parameter. For those with hardcoded height, both the > reference C function and the assembly implementations ignore the > parameter similarly. > > The documentation for the functions indicate that heights between > w/2 and 2*w, within the range of 4 to 16, should be supported. This > patch just tests random heights in that range, without knowing what > width the current function actually uses. > --- > I'm not sure if it's good to have checkasm exercise cases that > don't occur in practice or not. In particular, the aarch64 > functions have a separate implementation for non-multiple-of-4 > height, which probably doesn't ever get called in practice, while > other SIMD implementations lack that. > > Alternatively, we'd improve the documentation for the expectations > for these functions and make the test match that, and remove the > unused non-multiple-of-4 case in the aarch64 assembly. > --- > tests/checkasm/motion.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/tests/checkasm/motion.c b/tests/checkasm/motion.c > index 0112822174..79e4358941 100644 > --- a/tests/checkasm/motion.c > +++ b/tests/checkasm/motion.c > @@ -45,7 +45,7 @@ static void test_motion(const char *name, me_cmp_func test_func) > /* motion estimation can look up to 17 bytes ahead */ > static const int look_ahead = 17; > > - int i, x, y, d1, d2; > + int i, x, y, h, d1, d2; > uint8_t *ptr; > > LOCAL_ALIGNED_16(uint8_t, img1, [WIDTH * HEIGHT]); > @@ -68,14 +68,16 @@ static void test_motion(const char *name, me_cmp_func test_func) > for (i = 0; i < ITERATIONS; i++) { > x = rnd() % (WIDTH - look_ahead); > y = rnd() % (HEIGHT - look_ahead); > + // Pick a random h between 4 and 16; pick an even value. > + h = 4 + ((rnd() % (16 + 1 - 4)) & ~1); > > ptr = img2 + y * WIDTH + x; > - d2 = call_ref(NULL, img1, ptr, WIDTH, 8); > - d1 = call_new(NULL, img1, ptr, WIDTH, 8); > + d2 = call_ref(NULL, img1, ptr, WIDTH, h); > + d1 = call_new(NULL, img1, ptr, WIDTH, h); > > if (d1 != d2) { > fail(); > - printf("func: %s, x=%d y=%d, error: asm=%d c=%d\n", name, x, y, d1, d2); > + printf("func: %s, x=%d y=%d h=%d, error: asm=%d c=%d\n", name, x, y, h, d1, d2); > break; > } > } > -- > 2.25.1 Ping // Martin
On Thu, 4 Aug 2022, Martin Storsjö wrote: > On Wed, 13 Jul 2022, Martin Storsjö wrote: > >> Previously, the checkasm test always passed h=8, so no other cases >> were tested. >> >> Out of the me_cmp functions, in practice, some functions are hardcoded >> to always assume a 8x8 block (ignoring the h parameter), while others >> do use the parameter. For those with hardcoded height, both the >> reference C function and the assembly implementations ignore the >> parameter similarly. >> >> The documentation for the functions indicate that heights between >> w/2 and 2*w, within the range of 4 to 16, should be supported. This >> patch just tests random heights in that range, without knowing what >> width the current function actually uses. >> --- >> I'm not sure if it's good to have checkasm exercise cases that >> don't occur in practice or not. In particular, the aarch64 >> functions have a separate implementation for non-multiple-of-4 >> height, which probably doesn't ever get called in practice, while >> other SIMD implementations lack that. >> >> Alternatively, we'd improve the documentation for the expectations >> for these functions and make the test match that, and remove the >> unused non-multiple-of-4 case in the aarch64 assembly. >> --- >> tests/checkasm/motion.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/tests/checkasm/motion.c b/tests/checkasm/motion.c >> index 0112822174..79e4358941 100644 >> --- a/tests/checkasm/motion.c >> +++ b/tests/checkasm/motion.c >> @@ -45,7 +45,7 @@ static void test_motion(const char *name, me_cmp_func >> test_func) >> /* motion estimation can look up to 17 bytes ahead */ >> static const int look_ahead = 17; >> >> - int i, x, y, d1, d2; >> + int i, x, y, h, d1, d2; >> uint8_t *ptr; >> >> LOCAL_ALIGNED_16(uint8_t, img1, [WIDTH * HEIGHT]); >> @@ -68,14 +68,16 @@ static void test_motion(const char *name, me_cmp_func >> test_func) >> for (i = 0; i < ITERATIONS; i++) { >> x = rnd() % (WIDTH - look_ahead); >> y = rnd() % (HEIGHT - look_ahead); >> + // Pick a random h between 4 and 16; pick an even value. >> + h = 4 + ((rnd() % (16 + 1 - 4)) & ~1); >> >> ptr = img2 + y * WIDTH + x; >> - d2 = call_ref(NULL, img1, ptr, WIDTH, 8); >> - d1 = call_new(NULL, img1, ptr, WIDTH, 8); >> + d2 = call_ref(NULL, img1, ptr, WIDTH, h); >> + d1 = call_new(NULL, img1, ptr, WIDTH, h); >> >> if (d1 != d2) { >> fail(); >> - printf("func: %s, x=%d y=%d, error: asm=%d c=%d\n", name, >> x, y, d1, d2); >> + printf("func: %s, x=%d y=%d h=%d, error: asm=%d c=%d\n", >> name, x, y, h, d1, d2); >> break; >> } >> } >> -- >> 2.25.1 > > Ping As there doesn't seem to be much opinion on this, I'll go ahead and land this (patch 1/2 was approved already), as this improves the test coverage for the aarch64 neon me_cmp assembly patches that are in progress. // Martin
diff --git a/tests/checkasm/motion.c b/tests/checkasm/motion.c index 0112822174..79e4358941 100644 --- a/tests/checkasm/motion.c +++ b/tests/checkasm/motion.c @@ -45,7 +45,7 @@ static void test_motion(const char *name, me_cmp_func test_func) /* motion estimation can look up to 17 bytes ahead */ static const int look_ahead = 17; - int i, x, y, d1, d2; + int i, x, y, h, d1, d2; uint8_t *ptr; LOCAL_ALIGNED_16(uint8_t, img1, [WIDTH * HEIGHT]); @@ -68,14 +68,16 @@ static void test_motion(const char *name, me_cmp_func test_func) for (i = 0; i < ITERATIONS; i++) { x = rnd() % (WIDTH - look_ahead); y = rnd() % (HEIGHT - look_ahead); + // Pick a random h between 4 and 16; pick an even value. + h = 4 + ((rnd() % (16 + 1 - 4)) & ~1); ptr = img2 + y * WIDTH + x; - d2 = call_ref(NULL, img1, ptr, WIDTH, 8); - d1 = call_new(NULL, img1, ptr, WIDTH, 8); + d2 = call_ref(NULL, img1, ptr, WIDTH, h); + d1 = call_new(NULL, img1, ptr, WIDTH, h); if (d1 != d2) { fail(); - printf("func: %s, x=%d y=%d, error: asm=%d c=%d\n", name, x, y, d1, d2); + printf("func: %s, x=%d y=%d h=%d, error: asm=%d c=%d\n", name, x, y, h, d1, d2); break; } }