[PATCH] [inline assembly] fix pcm_dmix_i386.h assembly chunk interfaces
From: Frédéric Recoules frederic.recoules@orange.fr
Enabler change: - set HAVE_MMX in configure if the compiler is aware of MMX technology
Main changes are: - move 'size' and 'old_ebx' to the output list since they are clobbered - add the "memory" keyword since input pointers are dereferenced - add mmx registers in the clobber list and add an initialization for mm1 - add ebx in clobbers via a set of macro when GCC is newer than 5.0 (it will work for other compilers or non-PIC mode too)
Minor changes are: - keep consistent the token numbering in the template - remove the manual save/restore ebx when it is in the clobber list - allows 'dst_step', 'src_step' and 'sum_step' to be given by immediates - allows 'size' to be given by register (e.g. ebp) - add "cc" keyword since the eflag register is clobbered
Signed-off-by: Frédéric Recoules frederic.recoules@orange.fr --- configure.ac | 7 ++ src/pcm/pcm_dmix_i386.h | 174 +++++++++++++++++++++++----------------- 2 files changed, 106 insertions(+), 75 deletions(-)
diff --git a/configure.ac b/configure.ac index 4b5ab662..1838e50b 100644 --- a/configure.ac +++ b/configure.ac @@ -516,6 +516,13 @@ if test -z "$gcc_have_atomics"; then fi AC_MSG_RESULT($gcc_have_atomics)
+dnl check mmx register for pcm_dmix_i386 + +AC_TRY_LINK([], + [__asm__ volatile ("" : : : "mm0");], + [AC_DEFINE([HAVE_MMX], "1", [MMX technology is enabled])], + []) + PCM_PLUGIN_LIST="copy linear route mulaw alaw adpcm rate plug multi shm file null empty share meter hooks lfloat ladspa dmix dshare dsnoop asym iec958 softvol extplug ioplug mmap_emul"
build_pcm_plugin="no" diff --git a/src/pcm/pcm_dmix_i386.h b/src/pcm/pcm_dmix_i386.h index 2778cb1d..aa1717f6 100644 --- a/src/pcm/pcm_dmix_i386.h +++ b/src/pcm/pcm_dmix_i386.h @@ -26,6 +26,13 @@ * */
+#define COMMA , +#if __GNUC__ < 5 && defined(__PIC__) +# define GCC_PIC_SWITCH(before,after) before +#else +# define GCC_PIC_SWITCH(before,after) after +#endif + /* * for plain i386 */ @@ -47,13 +54,14 @@ static void MIX_AREAS_16(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */ + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",) /* * initialization, load ESI, EDI, EBX registers */ - "\tmovl %1, %%edi\n" - "\tmovl %2, %%esi\n" - "\tmovl %3, %%ebx\n" + "\tmovl %2, %%edi\n" + "\tmovl %3, %%esi\n" + "\tmovl %4, %%ebx\n" "\tcmpl $0, %0\n" "\tjnz 2f\n" "\tjmp 7f\n" @@ -64,9 +72,9 @@ static void MIX_AREAS_16(unsigned int size, */ "\t.p2align 4,,15\n" "1:" - "\tadd %4, %%edi\n" - "\tadd %5, %%esi\n" - "\tadd %6, %%ebx\n" + "\tadd %5, %%edi\n" + "\tadd %6, %%esi\n" + "\tadd %7, %%ebx\n"
/* * sample = *src; @@ -138,15 +146,16 @@ static void MIX_AREAS_16(unsigned int size, "\tjnz 4b\n" "\tdecl %0\n" "\tjnz 1b\n" - - "7:" - "\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
- : /* no output regs */ - : "m" (size), "m" (dst), "m" (src), - "m" (sum), "m" (dst_step), "m" (src_step), - "m" (sum_step), "m" (old_ebx) - : "esi", "edi", "edx", "ecx", "eax" + "7:" + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",) + + : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx) + : "m" (dst), "m" (src), "m" (sum), + "im" (dst_step), "im" (src_step), "im" (sum_step) + : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax", + "memory", "cc" ); }
@@ -171,22 +180,24 @@ static void MIX_AREAS_16_MMX(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */ + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",) /* - * initialization, load ESI, EDI, EBX registers + * initialization, load ESI, EDI, EBX registers, clear MM1 */ - "\tmovl %1, %%edi\n" - "\tmovl %2, %%esi\n" - "\tmovl %3, %%ebx\n" + "\tpxor %%mm1, %%mm1\n" + "\tmovl %2, %%edi\n" + "\tmovl %3, %%esi\n" + "\tmovl %4, %%ebx\n" "\tcmpl $0, %0\n" "\tjnz 2f\n" "\tjmp 5f\n"
"\t.p2align 4,,15\n" "1:" - "\tadd %4, %%edi\n" - "\tadd %5, %%esi\n" - "\tadd %6, %%ebx\n" + "\tadd %5, %%edi\n" + "\tadd %6, %%esi\n" + "\tadd %7, %%ebx\n"
"2:" /* @@ -230,13 +241,20 @@ static void MIX_AREAS_16_MMX(unsigned int size, "\tjnz 1b\n" "\temms\n" "5:" - "\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */ - - : /* no output regs */ - : "m" (size), "m" (dst), "m" (src), - "m" (sum), "m" (dst_step), "m" (src_step), - "m" (sum_step), "m" (old_ebx) - : "esi", "edi", "edx", "ecx", "eax" + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",) + + : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx) + : "m" (dst), "m" (src), "m" (sum), + "im" (dst_step), "im" (src_step), "im" (sum_step) + : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax", + "memory", "cc", +# ifdef HAVE_MMX + "mm0", "mm1" +# else + "st", "st(1)", "st(2)", "st(3)", + "st(4)", "st(5)", "st(6)", "st(7)" +# endif ); }
@@ -261,13 +279,14 @@ static void MIX_AREAS_32(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */ + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",) /* * initialization, load ESI, EDI, EBX registers */ - "\tmovl %1, %%edi\n" - "\tmovl %2, %%esi\n" - "\tmovl %3, %%ebx\n" + "\tmovl %2, %%edi\n" + "\tmovl %3, %%esi\n" + "\tmovl %4, %%ebx\n" "\tcmpl $0, %0\n" "\tjnz 1f\n" "\tjmp 6f\n" @@ -337,19 +356,20 @@ static void MIX_AREAS_32(unsigned int size, */ "\tdecl %0\n" "\tjz 6f\n" - "\tadd %4, %%edi\n" - "\tadd %5, %%esi\n" - "\tadd %6, %%ebx\n" + "\tadd %5, %%edi\n" + "\tadd %6, %%esi\n" + "\tadd %7, %%ebx\n" "\tjmp 1b\n" - - "6:" - "\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
- : /* no output regs */ - : "m" (size), "m" (dst), "m" (src), - "m" (sum), "m" (dst_step), "m" (src_step), - "m" (sum_step), "m" (old_ebx) - : "esi", "edi", "edx", "ecx", "eax" + "6:" + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",) + + : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx) + : "m" (dst), "m" (src), "m" (sum), + "im" (dst_step), "im" (src_step), "im" (sum_step) + : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax", + "memory", "cc" ); }
@@ -374,13 +394,14 @@ static void MIX_AREAS_24(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */ + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",) /* * initialization, load ESI, EDI, EBX registers */ - "\tmovl %1, %%edi\n" - "\tmovl %2, %%esi\n" - "\tmovl %3, %%ebx\n" + "\tmovl %2, %%edi\n" + "\tmovl %3, %%esi\n" + "\tmovl %4, %%ebx\n" "\tcmpl $0, %0\n" "\tjnz 1f\n" "\tjmp 6f\n" @@ -443,19 +464,20 @@ static void MIX_AREAS_24(unsigned int size, */ "\tdecl %0\n" "\tjz 6f\n" - "\tadd %4, %%edi\n" - "\tadd %5, %%esi\n" - "\tadd %6, %%ebx\n" + "\tadd %5, %%edi\n" + "\tadd %6, %%esi\n" + "\tadd %7, %%ebx\n" "\tjmp 1b\n" - - "6:" - "\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
- : /* no output regs */ - : "m" (size), "m" (dst), "m" (src), - "m" (sum), "m" (dst_step), "m" (src_step), - "m" (sum_step), "m" (old_ebx) - : "esi", "edi", "edx", "ecx", "eax" + "6:" + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",) + + : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx) + : "m" (dst), "m" (src), "m" (sum), + "im" (dst_step), "im" (src_step), "im" (sum_step) + : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax", + "memory", "cc" ); }
@@ -480,13 +502,14 @@ static void MIX_AREAS_24_CMOV(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */ + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",) /* * initialization, load ESI, EDI, EBX registers */ - "\tmovl %1, %%edi\n" - "\tmovl %2, %%esi\n" - "\tmovl %3, %%ebx\n" + "\tmovl %2, %%edi\n" + "\tmovl %3, %%esi\n" + "\tmovl %4, %%ebx\n" "\tcmpl $0, %0\n" "\tjz 6f\n"
@@ -541,19 +564,20 @@ static void MIX_AREAS_24_CMOV(unsigned int size, /* * while (size-- > 0) */ - "\tadd %4, %%edi\n" - "\tadd %5, %%esi\n" - "\tadd %6, %%ebx\n" + "\tadd %5, %%edi\n" + "\tadd %6, %%esi\n" + "\tadd %7, %%ebx\n" "\tdecl %0\n" "\tjnz 1b\n" - - "6:" - "\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
- : /* no output regs */ - : "m" (size), "m" (dst), "m" (src), - "m" (sum), "m" (dst_step), "m" (src_step), - "m" (sum_step), "m" (old_ebx) - : "esi", "edi", "edx", "ecx", "eax" + "6:" + /* ebx is GOT pointer (-fPIC) */ + GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",) + + : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx) + : "m" (dst), "m" (src), "m" (sum), + "im" (dst_step), "im" (src_step), "im" (sum_step) + : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax", + "memory", "cc" ); }
On Mon, 27 Apr 2020 18:57:07 +0200, frederic.recoules@univ-grenoble-alpes.fr wrote:
From: Frédéric Recoules frederic.recoules@orange.fr
Enabler change:
- set HAVE_MMX in configure if the compiler is aware of MMX technology
Main changes are:
- move 'size' and 'old_ebx' to the output list since they are clobbered
- add the "memory" keyword since input pointers are dereferenced
- add mmx registers in the clobber list and add an initialization for mm1
- add ebx in clobbers via a set of macro when GCC is newer than 5.0 (it will work for other compilers or non-PIC mode too)
Minor changes are:
- keep consistent the token numbering in the template
- remove the manual save/restore ebx when it is in the clobber list
- allows 'dst_step', 'src_step' and 'sum_step' to be given by immediates
- allows 'size' to be given by register (e.g. ebp)
- add "cc" keyword since the eflag register is clobbered
Signed-off-by: Frédéric Recoules frederic.recoules@orange.fr
Thanks. Now I confirmed that the build test passed.
But before merging the patch: have you actually tested the code on your machine and confirmed that it keeps working? If you've tested, it'd be helpful to mention about the test coverage, it's a good measure for judgment. Since it handles different compile flavors (with and without MMX support), I'd like to know whether it's actually tested.
BTW, when you resubmit a patch with the correction, please put "v2" or whatever the revision in the subject to tell from the earlier patches. Also, it'd be appreciated to list up summaries of changes between v1 and v2 in the patch description or after the separator line ("---") if you don't need to include that in the git commit log.
thanks,
Takashi
configure.ac | 7 ++ src/pcm/pcm_dmix_i386.h | 174 +++++++++++++++++++++++----------------- 2 files changed, 106 insertions(+), 75 deletions(-)
diff --git a/configure.ac b/configure.ac index 4b5ab662..1838e50b 100644 --- a/configure.ac +++ b/configure.ac @@ -516,6 +516,13 @@ if test -z "$gcc_have_atomics"; then fi AC_MSG_RESULT($gcc_have_atomics)
+dnl check mmx register for pcm_dmix_i386
+AC_TRY_LINK([],
- [__asm__ volatile ("" : : : "mm0");],
- [AC_DEFINE([HAVE_MMX], "1", [MMX technology is enabled])],
- [])
PCM_PLUGIN_LIST="copy linear route mulaw alaw adpcm rate plug multi shm file null empty share meter hooks lfloat ladspa dmix dshare dsnoop asym iec958 softvol extplug ioplug mmap_emul"
build_pcm_plugin="no" diff --git a/src/pcm/pcm_dmix_i386.h b/src/pcm/pcm_dmix_i386.h index 2778cb1d..aa1717f6 100644 --- a/src/pcm/pcm_dmix_i386.h +++ b/src/pcm/pcm_dmix_i386.h @@ -26,6 +26,13 @@
*/
+#define COMMA , +#if __GNUC__ < 5 && defined(__PIC__) +# define GCC_PIC_SWITCH(before,after) before +#else +# define GCC_PIC_SWITCH(before,after) after +#endif
/*
- for plain i386
*/ @@ -47,13 +54,14 @@ static void MIX_AREAS_16(unsigned int size, __asm__ __volatile__ ( "\n"
"\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)*/
- initialization, load ESI, EDI, EBX registers
"\tmovl %1, %%edi\n""\tmovl %2, %%esi\n""\tmovl %3, %%ebx\n"
"\tmovl %2, %%edi\n""\tmovl %3, %%esi\n""\tmovl %4, %%ebx\n"@@ -64,9 +72,9 @@ static void MIX_AREAS_16(unsigned int size, */ "\t.p2align 4,,15\n" "1:"
"\tadd %4, %%edi\n""\tadd %5, %%esi\n""\tadd %6, %%ebx\n"
"\tadd %5, %%edi\n""\tadd %6, %%esi\n""\tadd %7, %%ebx\n"/*
- sample = *src;
@@ -138,15 +146,16 @@ static void MIX_AREAS_16(unsigned int size, "\tjnz 4b\n" "\tdecl %0\n" "\tjnz 1b\n"
"7:""\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */: /* no output regs */: "m" (size), "m" (dst), "m" (src),"m" (sum), "m" (dst_step), "m" (src_step),"m" (sum_step), "m" (old_ebx): "esi", "edi", "edx", "ecx", "eax"
"7:"/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",): "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx): "m" (dst), "m" (src), "m" (sum),"im" (dst_step), "im" (src_step), "im" (sum_step): "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax","memory", "cc"}
@@ -171,22 +180,24 @@ static void MIX_AREAS_16_MMX(unsigned int size, __asm__ __volatile__ ( "\n"
"\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
* initialization, load ESI, EDI, EBX registers
* initialization, load ESI, EDI, EBX registers, clear MM1
"\tmovl %1, %%edi\n""\tmovl %2, %%esi\n""\tmovl %3, %%ebx\n"
"\tpxor %%mm1, %%mm1\n""\tmovl %2, %%edi\n""\tmovl %3, %%esi\n""\tmovl %4, %%ebx\n""\tcmpl $0, %0\n" "\tjnz 2f\n" "\tjmp 5f\n"
"\t.p2align 4,,15\n" "1:"
"\tadd %4, %%edi\n""\tadd %5, %%esi\n""\tadd %6, %%ebx\n"
"\tadd %5, %%edi\n""\tadd %6, %%esi\n""\tadd %7, %%ebx\n""2:" /*
@@ -230,13 +241,20 @@ static void MIX_AREAS_16_MMX(unsigned int size, "\tjnz 1b\n" "\temms\n" "5:"
"\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */: /* no output regs */: "m" (size), "m" (dst), "m" (src),"m" (sum), "m" (dst_step), "m" (src_step),"m" (sum_step), "m" (old_ebx): "esi", "edi", "edx", "ecx", "eax"
/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",): "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx): "m" (dst), "m" (src), "m" (sum),"im" (dst_step), "im" (src_step), "im" (sum_step): "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax","memory", "cc",+# ifdef HAVE_MMX
"mm0", "mm1"+# else
"st", "st(1)", "st(2)", "st(3)","st(4)", "st(5)", "st(6)", "st(7)"+# endif ); }
@@ -261,13 +279,14 @@ static void MIX_AREAS_32(unsigned int size, __asm__ __volatile__ ( "\n"
"\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)*/
- initialization, load ESI, EDI, EBX registers
"\tmovl %1, %%edi\n""\tmovl %2, %%esi\n""\tmovl %3, %%ebx\n"
"\tmovl %2, %%edi\n""\tmovl %3, %%esi\n""\tmovl %4, %%ebx\n"@@ -337,19 +356,20 @@ static void MIX_AREAS_32(unsigned int size, */ "\tdecl %0\n" "\tjz 6f\n"
"\tadd %4, %%edi\n""\tadd %5, %%esi\n""\tadd %6, %%ebx\n"
"\tadd %5, %%edi\n""\tadd %6, %%esi\n""\tadd %7, %%ebx\n"
"6:""\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */: /* no output regs */: "m" (size), "m" (dst), "m" (src),"m" (sum), "m" (dst_step), "m" (src_step),"m" (sum_step), "m" (old_ebx): "esi", "edi", "edx", "ecx", "eax"
"6:"/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",): "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx): "m" (dst), "m" (src), "m" (sum),"im" (dst_step), "im" (src_step), "im" (sum_step): "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax","memory", "cc"}
@@ -374,13 +394,14 @@ static void MIX_AREAS_24(unsigned int size, __asm__ __volatile__ ( "\n"
"\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)*/
- initialization, load ESI, EDI, EBX registers
"\tmovl %1, %%edi\n""\tmovl %2, %%esi\n""\tmovl %3, %%ebx\n"
"\tmovl %2, %%edi\n""\tmovl %3, %%esi\n""\tmovl %4, %%ebx\n"@@ -443,19 +464,20 @@ static void MIX_AREAS_24(unsigned int size, */ "\tdecl %0\n" "\tjz 6f\n"
"\tadd %4, %%edi\n""\tadd %5, %%esi\n""\tadd %6, %%ebx\n"
"\tadd %5, %%edi\n""\tadd %6, %%esi\n""\tadd %7, %%ebx\n"
"6:""\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */: /* no output regs */: "m" (size), "m" (dst), "m" (src),"m" (sum), "m" (dst_step), "m" (src_step),"m" (sum_step), "m" (old_ebx): "esi", "edi", "edx", "ecx", "eax"
"6:"/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",): "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx): "m" (dst), "m" (src), "m" (sum),"im" (dst_step), "im" (src_step), "im" (sum_step): "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax","memory", "cc"}
@@ -480,13 +502,14 @@ static void MIX_AREAS_24_CMOV(unsigned int size, __asm__ __volatile__ ( "\n"
"\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)*/
- initialization, load ESI, EDI, EBX registers
"\tmovl %1, %%edi\n""\tmovl %2, %%esi\n""\tmovl %3, %%ebx\n"
"\tmovl %2, %%edi\n""\tmovl %3, %%esi\n""\tmovl %4, %%ebx\n"@@ -541,19 +564,20 @@ static void MIX_AREAS_24_CMOV(unsigned int size, /* * while (size-- > 0) */
"\tadd %4, %%edi\n""\tadd %5, %%esi\n""\tadd %6, %%ebx\n"
"\tadd %5, %%edi\n""\tadd %6, %%esi\n""\tadd %7, %%ebx\n"
"6:""\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */: /* no output regs */: "m" (size), "m" (dst), "m" (src),"m" (sum), "m" (dst_step), "m" (src_step),"m" (sum_step), "m" (old_ebx): "esi", "edi", "edx", "ecx", "eax"
"6:"/* ebx is GOT pointer (-fPIC) */GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",): "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx): "m" (dst), "m" (src), "m" (sum),"im" (dst_step), "im" (src_step), "im" (sum_step): "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax","memory", "cc"}
2.17.1
Hi Takashi,
I resubmitted the patch (V2) with some modifications: - I split the changes so you will not have to apply all of them; - the 4 first patches are the ones for safety, and barely produce the same binary output some I am highly confident in the fact they do not need testing; - the 2 last patches are small changes that can help the compiler producing a better/smaller code. They have not been tested yet. Have you any ready to go benchmark to test with? - the patches work both on 32- and 64bit version.
To compare the binary output, I used objdump and diff on libpcm.a. I compared the master with each patch with both 32 and 64 versions.
Hope it will help. Regards, Frédéric
De: "tiwai" tiwai@suse.de À: "frederic recoules" frederic.recoules@univ-grenoble-alpes.fr Cc: "alsa-devel" alsa-devel@alsa-project.org, "frederic recoules" frederic.recoules@orange.fr Envoyé: Mercredi 29 Avril 2020 10:19:11 Objet: Re: [PATCH] [inline assembly] fix pcm_dmix_i386.h assembly chunk interfaces
On Mon, 27 Apr 2020 18:57:07 +0200, frederic.recoules@univ-grenoble-alpes.fr wrote:
From: Frédéric Recoules frederic.recoules@orange.fr
Enabler change:
- set HAVE_MMX in configure if the compiler is aware of MMX technology
Main changes are:
- move 'size' and 'old_ebx' to the output list since they are clobbered
- add the "memory" keyword since input pointers are dereferenced
- add mmx registers in the clobber list and add an initialization for mm1
- add ebx in clobbers via a set of macro when GCC is newer than 5.0
(it will work for other compilers or non-PIC mode too)
Minor changes are:
- keep consistent the token numbering in the template
- remove the manual save/restore ebx when it is in the clobber list
- allows 'dst_step', 'src_step' and 'sum_step' to be given by immediates
- allows 'size' to be given by register (e.g. ebp)
- add "cc" keyword since the eflag register is clobbered
Signed-off-by: Frédéric Recoules frederic.recoules@orange.fr
Thanks. Now I confirmed that the build test passed.
But before merging the patch: have you actually tested the code on your machine and confirmed that it keeps working? If you've tested, it'd be helpful to mention about the test coverage, it's a good measure for judgment. Since it handles different compile flavors (with and without MMX support), I'd like to know whether it's actually tested.
BTW, when you resubmit a patch with the correction, please put "v2" or whatever the revision in the subject to tell from the earlier patches. Also, it'd be appreciated to list up summaries of changes between v1 and v2 in the patch description or after the separator line ("---") if you don't need to include that in the git commit log.
thanks,
Takashi
configure.ac | 7 ++ src/pcm/pcm_dmix_i386.h | 174 +++++++++++++++++++++++----------------- 2 files changed, 106 insertions(+), 75 deletions(-)
diff --git a/configure.ac b/configure.ac index 4b5ab662..1838e50b 100644 --- a/configure.ac +++ b/configure.ac @@ -516,6 +516,13 @@ if test -z "$gcc_have_atomics"; then fi AC_MSG_RESULT($gcc_have_atomics)
+dnl check mmx register for pcm_dmix_i386
+AC_TRY_LINK([],
- [__asm__ volatile ("" : : : "mm0");],
- [AC_DEFINE([HAVE_MMX], "1", [MMX technology is enabled])],
- [])
PCM_PLUGIN_LIST="copy linear route mulaw alaw adpcm rate plug multi shm file null empty share meter hooks lfloat ladspa dmix dshare dsnoop asym iec958 softvol extplug ioplug mmap_emul"
build_pcm_plugin="no" diff --git a/src/pcm/pcm_dmix_i386.h b/src/pcm/pcm_dmix_i386.h index 2778cb1d..aa1717f6 100644 --- a/src/pcm/pcm_dmix_i386.h +++ b/src/pcm/pcm_dmix_i386.h @@ -26,6 +26,13 @@
*/
+#define COMMA , +#if __GNUC__ < 5 && defined(__PIC__) +# define GCC_PIC_SWITCH(before,after) before +#else +# define GCC_PIC_SWITCH(before,after) after +#endif
/*
- for plain i386
*/ @@ -47,13 +54,14 @@ static void MIX_AREAS_16(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjnz 2f\n" "\tjmp 7f\n" @@ -64,9 +72,9 @@ static void MIX_AREAS_16(unsigned int size, */ "\t.p2align 4,,15\n" "1:"
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
/*
- sample = *src;
@@ -138,15 +146,16 @@ static void MIX_AREAS_16(unsigned int size, "\tjnz 4b\n" "\tdecl %0\n" "\tjnz 1b\n"
"7:"
"\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
: /* no output regs */
: "m" (size), "m" (dst), "m" (src),
"m" (sum), "m" (dst_step), "m" (src_step),
"m" (sum_step), "m" (old_ebx)
: "esi", "edi", "edx", "ecx", "eax"
- "7:"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc"
); }
@@ -171,22 +180,24 @@ static void MIX_AREAS_16_MMX(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
- initialization, load ESI, EDI, EBX registers, clear MM1
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tpxor %%mm1, %%mm1\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjnz 2f\n" "\tjmp 5f\n"
"\t.p2align 4,,15\n" "1:"
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
"2:" /* @@ -230,13 +241,20 @@ static void MIX_AREAS_16_MMX(unsigned int size, "\tjnz 1b\n" "\temms\n" "5:"
- "\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
- : /* no output regs */
- : "m" (size), "m" (dst), "m" (src),
- "m" (sum), "m" (dst_step), "m" (src_step),
- "m" (sum_step), "m" (old_ebx)
- : "esi", "edi", "edx", "ecx", "eax"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc",
+# ifdef HAVE_MMX
- "mm0", "mm1"
+# else
- "st", "st(1)", "st(2)", "st(3)",
- "st(4)", "st(5)", "st(6)", "st(7)"
+# endif ); }
@@ -261,13 +279,14 @@ static void MIX_AREAS_32(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjnz 1f\n" "\tjmp 6f\n" @@ -337,19 +356,20 @@ static void MIX_AREAS_32(unsigned int size, */ "\tdecl %0\n" "\tjz 6f\n"
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
"\tjmp 1b\n"
"6:"
"\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
: /* no output regs */
: "m" (size), "m" (dst), "m" (src),
"m" (sum), "m" (dst_step), "m" (src_step),
"m" (sum_step), "m" (old_ebx)
: "esi", "edi", "edx", "ecx", "eax"
- "6:"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc"
); }
@@ -374,13 +394,14 @@ static void MIX_AREAS_24(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjnz 1f\n" "\tjmp 6f\n" @@ -443,19 +464,20 @@ static void MIX_AREAS_24(unsigned int size, */ "\tdecl %0\n" "\tjz 6f\n"
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
"\tjmp 1b\n"
"6:"
"\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
: /* no output regs */
: "m" (size), "m" (dst), "m" (src),
"m" (sum), "m" (dst_step), "m" (src_step),
"m" (sum_step), "m" (old_ebx)
: "esi", "edi", "edx", "ecx", "eax"
- "6:"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc"
); }
@@ -480,13 +502,14 @@ static void MIX_AREAS_24_CMOV(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjz 6f\n"
@@ -541,19 +564,20 @@ static void MIX_AREAS_24_CMOV(unsigned int size, /*
- while (size-- > 0)
*/
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
"\tdecl %0\n" "\tjnz 1b\n"
"6:"
"\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
: /* no output regs */
: "m" (size), "m" (dst), "m" (src),
"m" (sum), "m" (dst_step), "m" (src_step),
"m" (sum_step), "m" (old_ebx)
: "esi", "edi", "edx", "ecx", "eax"
- "6:"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc"
); } -- 2.17.1
Hi Takashi,
I would like an update on the review process ([PATCH */6 V2] [pcm_dmix assembly])
As a reminder, I split the changes in 6 distinct patches. The 3 first patches produce exactly the same binary output, so they do not need testing. The 4th one has just some minor change due to the fact that I added an instruction -- I am highly confident it breaks nothing.
If you need I test the 2 last ones (that reduce the size of the produced binary), could you point me out what test I should run?
Meanwhile, my deadline comes and I would really appreciate to see the patches applied by wednesday night.
Best regards, Frédéric
De: "FRÉDÉRIC RECOULES" frederic.recoules@univ-grenoble-alpes.fr À: "tiwai" tiwai@suse.de Cc: "alsa-devel" alsa-devel@alsa-project.org, "frederic recoules" frederic.recoules@orange.fr Envoyé: Jeudi 30 Avril 2020 11:41:56 Objet: Re: [PATCH] [inline assembly] fix pcm_dmix_i386.h assembly chunk interfaces
Hi Takashi,
I resubmitted the patch (V2) with some modifications: - I split the changes so you will not have to apply all of them; - the 4 first patches are the ones for safety, and barely produce the same binary output some I am highly confident in the fact they do not need testing; - the 2 last patches are small changes that can help the compiler producing a better/smaller code. They have not been tested yet. Have you any ready to go benchmark to test with? - the patches work both on 32- and 64bit version.
To compare the binary output, I used objdump and diff on libpcm.a. I compared the master with each patch with both 32 and 64 versions.
Hope it will help. Regards, Frédéric
De: "tiwai" tiwai@suse.de À: "frederic recoules" frederic.recoules@univ-grenoble-alpes.fr Cc: "alsa-devel" alsa-devel@alsa-project.org, "frederic recoules" frederic.recoules@orange.fr Envoyé: Mercredi 29 Avril 2020 10:19:11 Objet: Re: [PATCH] [inline assembly] fix pcm_dmix_i386.h assembly chunk interfaces
On Mon, 27 Apr 2020 18:57:07 +0200, frederic.recoules@univ-grenoble-alpes.fr wrote:
From: Frédéric Recoules frederic.recoules@orange.fr
Enabler change:
- set HAVE_MMX in configure if the compiler is aware of MMX technology
Main changes are:
- move 'size' and 'old_ebx' to the output list since they are clobbered
- add the "memory" keyword since input pointers are dereferenced
- add mmx registers in the clobber list and add an initialization for mm1
- add ebx in clobbers via a set of macro when GCC is newer than 5.0
(it will work for other compilers or non-PIC mode too)
Minor changes are:
- keep consistent the token numbering in the template
- remove the manual save/restore ebx when it is in the clobber list
- allows 'dst_step', 'src_step' and 'sum_step' to be given by immediates
- allows 'size' to be given by register (e.g. ebp)
- add "cc" keyword since the eflag register is clobbered
Signed-off-by: Frédéric Recoules frederic.recoules@orange.fr
Thanks. Now I confirmed that the build test passed.
But before merging the patch: have you actually tested the code on your machine and confirmed that it keeps working? If you've tested, it'd be helpful to mention about the test coverage, it's a good measure for judgment. Since it handles different compile flavors (with and without MMX support), I'd like to know whether it's actually tested.
BTW, when you resubmit a patch with the correction, please put "v2" or whatever the revision in the subject to tell from the earlier patches. Also, it'd be appreciated to list up summaries of changes between v1 and v2 in the patch description or after the separator line ("---") if you don't need to include that in the git commit log.
thanks,
Takashi
configure.ac | 7 ++ src/pcm/pcm_dmix_i386.h | 174 +++++++++++++++++++++++----------------- 2 files changed, 106 insertions(+), 75 deletions(-)
diff --git a/configure.ac b/configure.ac index 4b5ab662..1838e50b 100644 --- a/configure.ac +++ b/configure.ac @@ -516,6 +516,13 @@ if test -z "$gcc_have_atomics"; then fi AC_MSG_RESULT($gcc_have_atomics)
+dnl check mmx register for pcm_dmix_i386
+AC_TRY_LINK([],
- [__asm__ volatile ("" : : : "mm0");],
- [AC_DEFINE([HAVE_MMX], "1", [MMX technology is enabled])],
- [])
PCM_PLUGIN_LIST="copy linear route mulaw alaw adpcm rate plug multi shm file null empty share meter hooks lfloat ladspa dmix dshare dsnoop asym iec958 softvol extplug ioplug mmap_emul"
build_pcm_plugin="no" diff --git a/src/pcm/pcm_dmix_i386.h b/src/pcm/pcm_dmix_i386.h index 2778cb1d..aa1717f6 100644 --- a/src/pcm/pcm_dmix_i386.h +++ b/src/pcm/pcm_dmix_i386.h @@ -26,6 +26,13 @@
*/
+#define COMMA , +#if __GNUC__ < 5 && defined(__PIC__) +# define GCC_PIC_SWITCH(before,after) before +#else +# define GCC_PIC_SWITCH(before,after) after +#endif
/*
- for plain i386
*/ @@ -47,13 +54,14 @@ static void MIX_AREAS_16(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjnz 2f\n" "\tjmp 7f\n" @@ -64,9 +72,9 @@ static void MIX_AREAS_16(unsigned int size, */ "\t.p2align 4,,15\n" "1:"
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
/*
- sample = *src;
@@ -138,15 +146,16 @@ static void MIX_AREAS_16(unsigned int size, "\tjnz 4b\n" "\tdecl %0\n" "\tjnz 1b\n"
"7:"
"\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
: /* no output regs */
: "m" (size), "m" (dst), "m" (src),
"m" (sum), "m" (dst_step), "m" (src_step),
"m" (sum_step), "m" (old_ebx)
: "esi", "edi", "edx", "ecx", "eax"
- "7:"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc"
); }
@@ -171,22 +180,24 @@ static void MIX_AREAS_16_MMX(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
- initialization, load ESI, EDI, EBX registers, clear MM1
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tpxor %%mm1, %%mm1\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjnz 2f\n" "\tjmp 5f\n"
"\t.p2align 4,,15\n" "1:"
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
"2:" /* @@ -230,13 +241,20 @@ static void MIX_AREAS_16_MMX(unsigned int size, "\tjnz 1b\n" "\temms\n" "5:"
- "\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
- : /* no output regs */
- : "m" (size), "m" (dst), "m" (src),
- "m" (sum), "m" (dst_step), "m" (src_step),
- "m" (sum_step), "m" (old_ebx)
- : "esi", "edi", "edx", "ecx", "eax"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc",
+# ifdef HAVE_MMX
- "mm0", "mm1"
+# else
- "st", "st(1)", "st(2)", "st(3)",
- "st(4)", "st(5)", "st(6)", "st(7)"
+# endif ); }
@@ -261,13 +279,14 @@ static void MIX_AREAS_32(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjnz 1f\n" "\tjmp 6f\n" @@ -337,19 +356,20 @@ static void MIX_AREAS_32(unsigned int size, */ "\tdecl %0\n" "\tjz 6f\n"
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
"\tjmp 1b\n"
"6:"
"\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
: /* no output regs */
: "m" (size), "m" (dst), "m" (src),
"m" (sum), "m" (dst_step), "m" (src_step),
"m" (sum_step), "m" (old_ebx)
: "esi", "edi", "edx", "ecx", "eax"
- "6:"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc"
); }
@@ -374,13 +394,14 @@ static void MIX_AREAS_24(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjnz 1f\n" "\tjmp 6f\n" @@ -443,19 +464,20 @@ static void MIX_AREAS_24(unsigned int size, */ "\tdecl %0\n" "\tjz 6f\n"
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
"\tjmp 1b\n"
"6:"
"\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
: /* no output regs */
: "m" (size), "m" (dst), "m" (src),
"m" (sum), "m" (dst_step), "m" (src_step),
"m" (sum_step), "m" (old_ebx)
: "esi", "edi", "edx", "ecx", "eax"
- "6:"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc"
); }
@@ -480,13 +502,14 @@ static void MIX_AREAS_24_CMOV(unsigned int size, __asm__ __volatile__ ( "\n"
- "\tmovl %%ebx, %7\n" /* ebx is GOT pointer (-fPIC) */
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %%ebx, %1\n",)
/*
- initialization, load ESI, EDI, EBX registers
*/
- "\tmovl %1, %%edi\n"
- "\tmovl %2, %%esi\n"
- "\tmovl %3, %%ebx\n"
- "\tmovl %2, %%edi\n"
- "\tmovl %3, %%esi\n"
- "\tmovl %4, %%ebx\n"
"\tcmpl $0, %0\n" "\tjz 6f\n"
@@ -541,19 +564,20 @@ static void MIX_AREAS_24_CMOV(unsigned int size, /*
- while (size-- > 0)
*/
- "\tadd %4, %%edi\n"
- "\tadd %5, %%esi\n"
- "\tadd %6, %%ebx\n"
- "\tadd %5, %%edi\n"
- "\tadd %6, %%esi\n"
- "\tadd %7, %%ebx\n"
"\tdecl %0\n" "\tjnz 1b\n"
"6:"
"\tmovl %7, %%ebx\n" /* ebx is GOT pointer (-fPIC) */
: /* no output regs */
: "m" (size), "m" (dst), "m" (src),
"m" (sum), "m" (dst_step), "m" (src_step),
"m" (sum_step), "m" (old_ebx)
: "esi", "edi", "edx", "ecx", "eax"
- "6:"
- /* ebx is GOT pointer (-fPIC) */
- GCC_PIC_SWITCH("\tmovl %1, %%ebx\n",)
- : "+&rm" (size), GCC_PIC_SWITCH("=m","=X") (old_ebx)
- : "m" (dst), "m" (src), "m" (sum),
- "im" (dst_step), "im" (src_step), "im" (sum_step)
- : "esi", "edi", "edx", "ecx", GCC_PIC_SWITCH(,"ebx"COMMA) "eax",
- "memory", "cc"
); } -- 2.17.1
On Mon, 04 May 2020 21:25:16 +0200, FRÉDÉRIC RECOULES wrote:
Hi Takashi,
I would like an update on the review process ([PATCH */6 V2] [pcm_dmix assembly])
As a reminder, I split the changes in 6 distinct patches. The 3 first patches produce exactly the same binary output, so they do not need testing. The 4th one has just some minor change due to the fact that I added an instruction -- I am highly confident it breaks nothing.
The compile tests passed with a few different compiler set, so that's good. But there were some issues with the patch format. IIRC, the patch 2 couldn't be applied to the latest git tree cleanly (some space letter issues?), so I had to manually modify it.
Also, some style issues:
- Please avoid a prefix like "[configure]" in the subject. The prefix with "[PATCH xxx]" is good, and this should remain, but the next prefix should be "configure:" instead. Otherwise the prefix with the brackets are pruned at applying a patch via git-am.
- Please give more texts about why the change is done. In all your patches, there are no explanations why you change it. It's often more important than describing what you're changing. For example, the patch 2 "change the token by symbolic names". Why is this needed to be symbolic names? Write some more information in each patch description.
- We usually use #ifdef without space between "#" and "ifdef". Let's keep that style consistently.
If you need I test the 2 last ones (that reduce the size of the produced binary), could you point me out what test I should run?
We need at least some build tests with different compiler versions and check whether dmix actually works (not necessarily on all of them but some of those compiled results).
Meanwhile, my deadline comes and I would really appreciate to see the patches applied by wednesday night.
If you can work on the above and resubmit v3 patchset, I'll happily apply them.
Thanks!
Takashi
Hi Takashi,
Thanks for your comments. I addressed the style issues and I resubmitted the patches (V3).
Note about the MMX patch: The mm1 register is read by the chunk, but its value is not used (the instruction packssdw put the value of mm1 in the high 32 bits of mm0 but then, only the low 32 bits are used. My tool made an over-approximation but it is now fixed).
By the way, the first 3 patches produce the same binary output as master. However, I looked in the 'test' folder, but I do not know how to run a test for the pcm. For now, I have no time to investigate how alsa should be run till the next week.
But there were some issues with the patch format. IIRC, the patch 2 couldn't be applied to the latest git tree cleanly (some space etter issues?), so I had to manually modify it.
Sorry for that, it looks like my text editor remove space at the end of the line automatically. It should not be the case with the new patches.
Regards, Frédéric
De: "tiwai" tiwai@suse.de À: "FRÉDÉRIC RECOULES" frederic.recoules@univ-grenoble-alpes.fr Cc: "alsa-devel" alsa-devel@alsa-project.org, "frederic recoules" frederic.recoules@orange.fr Envoyé: Mardi 5 Mai 2020 15:52:19 Objet: Re: [PATCH] [inline assembly] fix pcm_dmix_i386.h assembly chunk interfaces
On Mon, 04 May 2020 21:25:16 +0200, FRÉDÉRIC RECOULES wrote:
Hi Takashi,
I would like an update on the review process ([PATCH */6 V2] [pcm_dmix assembly])
As a reminder, I split the changes in 6 distinct patches. The 3 first patches produce exactly the same binary output, so they do not need testing. The 4th one has just some minor change due to the fact that I added an instruction -- I am highly confident it breaks nothing.
The compile tests passed with a few different compiler set, so that's good. But there were some issues with the patch format. IIRC, the patch 2 couldn't be applied to the latest git tree cleanly (some space letter issues?), so I had to manually modify it.
Also, some style issues:
- Please avoid a prefix like "[configure]" in the subject. The prefix with "[PATCH xxx]" is good, and this should remain, but the next prefix should be "configure:" instead. Otherwise the prefix with the brackets are pruned at applying a patch via git-am.
- Please give more texts about why the change is done. In all your patches, there are no explanations why you change it. It's often more important than describing what you're changing. For example, the patch 2 "change the token by symbolic names". Why is this needed to be symbolic names? Write some more information in each patch description.
- We usually use #ifdef without space between "#" and "ifdef". Let's keep that style consistently.
If you need I test the 2 last ones (that reduce the size of the produced binary), could you point me out what test I should run?
We need at least some build tests with different compiler versions and check whether dmix actually works (not necessarily on all of them but some of those compiled results).
Meanwhile, my deadline comes and I would really appreciate to see the patches applied by wednesday night.
If you can work on the above and resubmit v3 patchset, I'll happily apply them.
Thanks!
Takashi
participants (3)
-
frederic.recoules＠univ-grenoble-alpes.fr -
FRÉDÉRIC RECOULES
-
Takashi Iwai