| Message ID | 20240507150205.2039-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] checkasm/blockdsp: don't randomize the buffers for fill_block_tab | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
James Almer: > The requirement is either 8 or 16 bytes alignment, not 32. > This should help finding bugs in asm implementations. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > tests/checkasm/blockdsp.c | 23 +++++++++-------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/tests/checkasm/blockdsp.c b/tests/checkasm/blockdsp.c > index ab87fc8fa4..f67a38d302 100644 > --- a/tests/checkasm/blockdsp.c > +++ b/tests/checkasm/blockdsp.c > @@ -29,11 +29,6 @@ > #include "libavutil/intreadwrite.h" > #include "libavutil/mem_internal.h" > > -typedef struct { > - const char *name; > - int size; > -} test; > - > #define randomize_buffers(size) \ > do { \ > int i; \ > @@ -58,18 +53,18 @@ do { \ > } while (0) > > static void check_fill(BlockDSPContext *h){ > - const test tests[] = { > - {"fill_block_tab[0]", 16}, > - {"fill_block_tab[1]", 8}, > - }; > - LOCAL_ALIGNED_32(uint8_t, buf0, [16 * 16]); > - LOCAL_ALIGNED_32(uint8_t, buf1, [16 * 16]); > + LOCAL_ALIGNED_16(uint8_t, buf0_16, [16 * 16]); > + LOCAL_ALIGNED_16(uint8_t, buf1_16, [16 * 16]); > + LOCAL_ALIGNED_8(uint8_t, buf0_8, [8 * 8]); > + LOCAL_ALIGNED_8(uint8_t, buf1_8, [8 * 8]); > > - for (size_t t = 0; t < FF_ARRAY_ELEMS(tests); ++t) { > - int n = tests[t].size; > + for (int t = 0; t < 2; ++t) { > + uint8_t *buf0 = t ? buf0_8 : buf0_16; > + uint8_t *buf1 = t ? buf1_8 : buf1_16; > + int n = 16 - 8 * t; > declare_func(void, uint8_t *block, uint8_t value, > ptrdiff_t line_size, int h); > - if (check_func(h->fill_block_tab[t], "blockdsp.%s", tests[t].name)) { > + if (check_func(h->fill_block_tab[t], "blockdsp.fill_block_tab[%d]", t)) { > uint8_t value = rnd(); > memset(buf0, 0, sizeof(*buf0) * n * n); > memset(buf1, 0, sizeof(*buf0) * n * n); 1. I wouldn't be surprised if the *_8 buffers were still 16 byte aligned. You should probably force 8 byte alignment by using a 16 byte-aligned buffer with an offset of eight. 2. Can you also extend this test to actually test the case of stride != width? (And negative strides.) - Andreas
On 5/7/2024 12:14 PM, Andreas Rheinhardt wrote: > James Almer: >> The requirement is either 8 or 16 bytes alignment, not 32. >> This should help finding bugs in asm implementations. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> tests/checkasm/blockdsp.c | 23 +++++++++-------------- >> 1 file changed, 9 insertions(+), 14 deletions(-) >> >> diff --git a/tests/checkasm/blockdsp.c b/tests/checkasm/blockdsp.c >> index ab87fc8fa4..f67a38d302 100644 >> --- a/tests/checkasm/blockdsp.c >> +++ b/tests/checkasm/blockdsp.c >> @@ -29,11 +29,6 @@ >> #include "libavutil/intreadwrite.h" >> #include "libavutil/mem_internal.h" >> >> -typedef struct { >> - const char *name; >> - int size; >> -} test; >> - >> #define randomize_buffers(size) \ >> do { \ >> int i; \ >> @@ -58,18 +53,18 @@ do { \ >> } while (0) >> >> static void check_fill(BlockDSPContext *h){ >> - const test tests[] = { >> - {"fill_block_tab[0]", 16}, >> - {"fill_block_tab[1]", 8}, >> - }; >> - LOCAL_ALIGNED_32(uint8_t, buf0, [16 * 16]); >> - LOCAL_ALIGNED_32(uint8_t, buf1, [16 * 16]); >> + LOCAL_ALIGNED_16(uint8_t, buf0_16, [16 * 16]); >> + LOCAL_ALIGNED_16(uint8_t, buf1_16, [16 * 16]); >> + LOCAL_ALIGNED_8(uint8_t, buf0_8, [8 * 8]); >> + LOCAL_ALIGNED_8(uint8_t, buf1_8, [8 * 8]); >> >> - for (size_t t = 0; t < FF_ARRAY_ELEMS(tests); ++t) { >> - int n = tests[t].size; >> + for (int t = 0; t < 2; ++t) { >> + uint8_t *buf0 = t ? buf0_8 : buf0_16; >> + uint8_t *buf1 = t ? buf1_8 : buf1_16; >> + int n = 16 - 8 * t; >> declare_func(void, uint8_t *block, uint8_t value, >> ptrdiff_t line_size, int h); >> - if (check_func(h->fill_block_tab[t], "blockdsp.%s", tests[t].name)) { >> + if (check_func(h->fill_block_tab[t], "blockdsp.fill_block_tab[%d]", t)) { >> uint8_t value = rnd(); >> memset(buf0, 0, sizeof(*buf0) * n * n); >> memset(buf1, 0, sizeof(*buf0) * n * n); > > 1. I wouldn't be surprised if the *_8 buffers were still 16 byte > aligned. You should probably force 8 byte alignment by using a 16 > byte-aligned buffer with an offset of eight. Amended the following locally: > diff --git a/tests/checkasm/blockdsp.c b/tests/checkasm/blockdsp.c > index 8c1f8281d2..5f4d46b8fa 100644 > --- a/tests/checkasm/blockdsp.c > +++ b/tests/checkasm/blockdsp.c > @@ -55,12 +55,10 @@ do { \ > static void check_fill(BlockDSPContext *h){ > LOCAL_ALIGNED_16(uint8_t, buf0_16, [16 * 16]); > LOCAL_ALIGNED_16(uint8_t, buf1_16, [16 * 16]); > - LOCAL_ALIGNED_8(uint8_t, buf0_8, [8 * 8]); > - LOCAL_ALIGNED_8(uint8_t, buf1_8, [8 * 8]); > > for (int t = 0; t < 2; ++t) { > - uint8_t *buf0 = t ? buf0_8 : buf0_16; > - uint8_t *buf1 = t ? buf1_8 : buf1_16; > + uint8_t *buf0 = buf0_16 + t * /* force 8 byte alignment */ 8; > + uint8_t *buf1 = buf1_16 + t * /* force 8 byte alignment */ 8; > int n = 16 - 8 * t; > declare_func(void, uint8_t *block, uint8_t value, > ptrdiff_t line_size, int h); > 2. Can you also extend this test to actually test the case of stride != > width? (And negative strides.) Maybe later.
diff --git a/tests/checkasm/blockdsp.c b/tests/checkasm/blockdsp.c index ab87fc8fa4..f67a38d302 100644 --- a/tests/checkasm/blockdsp.c +++ b/tests/checkasm/blockdsp.c @@ -29,11 +29,6 @@ #include "libavutil/intreadwrite.h" #include "libavutil/mem_internal.h" -typedef struct { - const char *name; - int size; -} test; - #define randomize_buffers(size) \ do { \ int i; \ @@ -58,18 +53,18 @@ do { \ } while (0) static void check_fill(BlockDSPContext *h){ - const test tests[] = { - {"fill_block_tab[0]", 16}, - {"fill_block_tab[1]", 8}, - }; - LOCAL_ALIGNED_32(uint8_t, buf0, [16 * 16]); - LOCAL_ALIGNED_32(uint8_t, buf1, [16 * 16]); + LOCAL_ALIGNED_16(uint8_t, buf0_16, [16 * 16]); + LOCAL_ALIGNED_16(uint8_t, buf1_16, [16 * 16]); + LOCAL_ALIGNED_8(uint8_t, buf0_8, [8 * 8]); + LOCAL_ALIGNED_8(uint8_t, buf1_8, [8 * 8]); - for (size_t t = 0; t < FF_ARRAY_ELEMS(tests); ++t) { - int n = tests[t].size; + for (int t = 0; t < 2; ++t) { + uint8_t *buf0 = t ? buf0_8 : buf0_16; + uint8_t *buf1 = t ? buf1_8 : buf1_16; + int n = 16 - 8 * t; declare_func(void, uint8_t *block, uint8_t value, ptrdiff_t line_size, int h); - if (check_func(h->fill_block_tab[t], "blockdsp.%s", tests[t].name)) { + if (check_func(h->fill_block_tab[t], "blockdsp.fill_block_tab[%d]", t)) { uint8_t value = rnd(); memset(buf0, 0, sizeof(*buf0) * n * n); memset(buf1, 0, sizeof(*buf0) * n * n);
The requirement is either 8 or 16 bytes alignment, not 32. This should help finding bugs in asm implementations. Signed-off-by: James Almer <jamrial@gmail.com> --- tests/checkasm/blockdsp.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)