Re: [MPlayer-dev-eng] [PATCH] Make mp3lib SIMD optimizations work on AMD64, Part 5
Hi, 2007/5/20, Zuxy Meng <zuxy.meng@gmail.com>:
As discussed with Guillaume on IRC, I'll split my previous big patch (Rewrite synth_1to1_MMX....) into several small parts for easier review. Here's the first one, rewriting the generic code in synth_1to1_MMX from assembly to C, so we don't need to deal with different ABIs. I've tested it and confirmed it doesn't hurt performance.
Note I removed a conditional jump in the remaining assembly too. By analyzing the code I'm sure it's never taken so don't worry about that. Strictly speaking it should be in a seperate patch but then this patch would break mplayer...
Part 2 will replace 32-bit leal to equivalent add/sub (without the 'l' suffix) so pointer arithmetic will be 64-bit under amd64.
Part 3 will remove hardcoded registers.
Part 4 will kill tabinit_mmx.c. We don't need to compute the table at runtime; it can be predetermined.
Part 5 will correct data types, replacing 'long' with 'int' where necessary.
The last patch will deal with Makefile and macros.
We've reached Part 5. This is again a big diff, replacing many 'long' to 'int', and several 'real' to 'short'. Among those, many are necessary because under LP64 environment, sizeof(long) != sizeof(int) and sizeof(long) != sizeof(float); others are optimizations simply because we really don't need such a long type, or the declaration didn't match its use (declared as float, used as short). -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hi, Zuxy Meng wrote:
Hi,
2007/5/20, Zuxy Meng <zuxy.meng@gmail.com>:
As discussed with Guillaume on IRC, I'll split my previous big patch (Rewrite synth_1to1_MMX....) into several small parts for easier review.
[..]
Part 5 will correct data types, replacing 'long' with 'int' where necessary.
The last patch will deal with Makefile and macros.
We've reached Part 5. This is again a big diff, replacing many 'long' to 'int',
These look Ok, though this certainly deserves to be tested on x86, x86-64, and PPC (other archs welcome).
and several 'real' to 'short'.
These look fishy. For sure, "real" should be replaced by a standard C type: a simple grep shows: dct64_altivec.c:#define real float dct64_sse.c:typedef float real; decode_i586.c:#define real float /* ugly - but only way */ dct64_k7.c:#define real float /* ugly - but only way */ dct64_k7.c:#define real float /* ugly - but only way */ sr1.c:#define real float dct64_3dnow.c:#define real float /* ugly - but only way */ dct36_3dnow.c:#define real float /* ugly - but only way */ decode_MMX.c:#define real float /* ugly - but only way */ mpg123.h:#define REAL_IS_FLOAT mpg123.h:# define real float mpg123.h:# define real long double mpg123.h:# define real double ---> these aren't used, or are commented out. IMHO, this barbarism should be dealt with in a separate patch that simply s/real/float/g because as far as I can seen this is a leftover of an old version of the library.
Among those, many are necessary because under LP64 environment, sizeof(long) != sizeof(int) and sizeof(long) != sizeof(float); others are optimizations simply because we really don't need such a long type,
These look OK, but need to be tested on all major archs, as I wrote above.
or the declaration didn't match its use (declared as float, used as short).
This should be dealt with later on IMHO; after all, are they really necessary to add support for SIMD optimizations on AMD64? I'd be tempted to say no. Also, if you are going down this road, the other SIMD optimizations should be modified in a similar way, i.e. Altivec optimizations should be "fixed" too. Guillaume Guillaume
Hi, 2007/5/25, Guillaume Poirier <gpoirier@mplayerhq.hu>:
Hi,
Zuxy Meng wrote:
Hi,
2007/5/20, Zuxy Meng <zuxy.meng@gmail.com>:
As discussed with Guillaume on IRC, I'll split my previous big patch (Rewrite synth_1to1_MMX....) into several small parts for easier review.
[..]
Part 5 will correct data types, replacing 'long' with 'int' where necessary.
The last patch will deal with Makefile and macros.
We've reached Part 5. This is again a big diff, replacing many 'long' to 'int',
These look Ok, though this certainly deserves to be tested on x86, x86-64, and PPC (other archs welcome).
and several 'real' to 'short'.
These look fishy. For sure, "real" should be replaced by a standard C type:
a simple grep shows:
dct64_altivec.c:#define real float dct64_sse.c:typedef float real; decode_i586.c:#define real float /* ugly - but only way */ dct64_k7.c:#define real float /* ugly - but only way */ dct64_k7.c:#define real float /* ugly - but only way */
sr1.c:#define real float dct64_3dnow.c:#define real float /* ugly - but only way */ dct36_3dnow.c:#define real float /* ugly - but only way */ decode_MMX.c:#define real float /* ugly - but only way */
mpg123.h:#define REAL_IS_FLOAT mpg123.h:# define real float mpg123.h:# define real long double mpg123.h:# define real double ---> these aren't used, or are commented out.
IMHO, this barbarism should be dealt with in a separate patch that simply s/real/float/g because as far as I can seen this is a leftover of an old version of the library.
I agree.
Among those, many are necessary because under LP64 environment, sizeof(long) != sizeof(int) and sizeof(long) != sizeof(float); others are optimizations simply because we really don't need such a long type,
These look OK, but need to be tested on all major archs, as I wrote above.
I've tested on IA-32 and am waiting for your results in AMD64:-)
or the declaration didn't match its use (declared as float, used as short).
This should be dealt with later on IMHO; after all, are they really necessary to add support for SIMD optimizations on AMD64? I'd be tempted to say no. Also, if you are going down this road, the other SIMD optimizations should be modified in a similar way, i.e. Altivec optimizations should be "fixed" too.
Altivec version doesn't have this problem. Anyway I'll split the patch as you suggested. -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hello, On Fri, May 25, 2007 at 02:14:49PM +0800, Zuxy Meng wrote:
We've reached Part 5. This is again a big diff, replacing many 'long' to 'int', and several 'real' to 'short'. Among those, many are necessary because under LP64 environment, sizeof(long) != sizeof(int) and sizeof(long) != sizeof(float); others are optimizations simply because we really don't need such a long type, or the declaration didn't match its use (declared as float, used as short).
These are separate issues and should be fixed in different patches. Also if you changed long to int because the code assumes 32 bits, it would make more sense to change it to int32_t. Replacing long with int without thinking can make the code even slower on 64 bit systems. [...]
Index: mp3lib/sr1.c =================================================================== --- mp3lib/sr1.c ?????? 23383?? +++ mp3lib/sr1.c ???????????? @@ -50,7 +50,6 @@ int MP3_channels=0; int MP3_bps=2;
-static long outscale = 32768; #include "tabinit.c"
#if 1
@@ -413,7 +412,7 @@ _has_mmx = 0; dct36_func = dct36;
- make_decode_tables(outscale); + make_decode_tables();
#ifdef CAN_COMPILE_X86_ASM
[...]
@@ -36,9 +36,9 @@ 64019, 65290, 66494, 67629, 68692, 69679, 70590, 71420, 72169, 72835, 73415, 73908, 74313, 74630, 74856, 74992, 75038 };
-static void make_decode_tables(long scaleval) +static void make_decode_tables() { - int i,j,k,kr,divv; + int i,j,k,kr,divv,scaleval=32768;
This is unrelated and also completely useless since a static function that is used in only one place will be inlined anyway. In addition it is wrong since "static void make_decode_tables()" != "static void make_decode_tables(void)" Greetings, Reimar Döffinger
Hi, Reimar Döffinger wrote:
Hello, On Fri, May 25, 2007 at 02:14:49PM +0800, Zuxy Meng wrote:
We've reached Part 5. This is again a big diff, replacing many 'long' to 'int', and several 'real' to 'short'. Among those, many are necessary because under LP64 environment, sizeof(long) != sizeof(int) and sizeof(long) != sizeof(float); others are optimizations simply because we really don't need such a long type, or the declaration didn't match its use (declared as float, used as short).
These are separate issues and should be fixed in different patches. Also if you changed long to int because the code assumes 32 bits, it would make more sense to change it to int32_t. Replacing long with int without thinking can make the code even slower on 64 bit systems.
[..]
This is unrelated and also completely useless since a static function that is used in only one place will be inlined anyway. In addition it is wrong since "static void make_decode_tables()" != "static void make_decode_tables(void)"
Thanks for your quality review Reimar! I realize I'm quite a lousy reviewer, but I'm trying to do my best! :-( Guillaume
Hi, 2007/5/25, Guillaume Poirier <gpoirier@mplayerhq.hu>:
Hi,
Reimar Döffinger wrote:
Hello, On Fri, May 25, 2007 at 02:14:49PM +0800, Zuxy Meng wrote:
We've reached Part 5. This is again a big diff, replacing many 'long' to 'int', and several 'real' to 'short'. Among those, many are necessary because under LP64 environment, sizeof(long) != sizeof(int) and sizeof(long) != sizeof(float); others are optimizations simply because we really don't need such a long type, or the declaration didn't match its use (declared as float, used as short).
These are separate issues and should be fixed in different patches. Also if you changed long to int because the code assumes 32 bits, it would make more sense to change it to int32_t. Replacing long with int without thinking can make the code even slower on 64 bit systems.
[..]
This is unrelated and also completely useless since a static function that is used in only one place will be inlined anyway. In addition it is wrong since "static void make_decode_tables()" != "static void make_decode_tables(void)"
Thanks for your quality review Reimar! I realize I'm quite a lousy reviewer, but I'm trying to do my best! :-(
But you're always a prompt tester and more importantly, a nice guy who's easy to get along with:-) Anway I've updated the patch according to suggestions from you and Reimar, which should be cleaner. In this patch, long->int conversions are carried out because any of the following: 1. Assumed 32-bit access either by inline assembly or by bswap_32 macros. (e.g. variables in decode_i586.c, costab_mmx in decode_MMX.c and newhead in sr1.c) 2. Value that never exceed 32 bits. (such as variables representing sampling frequency and frame size, variables initialized to known 32-bit integers, and variables that are assigned by an 'int' and are used to pass an 'int' parameter. -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hi, 2007/5/25, Zuxy Meng <zuxy.meng@gmail.com>:
Hi,
2007/5/25, Guillaume Poirier <gpoirier@mplayerhq.hu>:
Hi,
Reimar Döffinger wrote:
Hello, On Fri, May 25, 2007 at 02:14:49PM +0800, Zuxy Meng wrote:
We've reached Part 5. This is again a big diff, replacing many 'long' to 'int', and several 'real' to 'short'. Among those, many are necessary because under LP64 environment, sizeof(long) != sizeof(int) and sizeof(long) != sizeof(float); others are optimizations simply because we really don't need such a long type, or the declaration didn't match its use (declared as float, used as short).
These are separate issues and should be fixed in different patches. Also if you changed long to int because the code assumes 32 bits, it would make more sense to change it to int32_t. Replacing long with int without thinking can make the code even slower on 64 bit systems.
[..]
This is unrelated and also completely useless since a static function that is used in only one place will be inlined anyway. In addition it is wrong since "static void make_decode_tables()" != "static void make_decode_tables(void)"
Thanks for your quality review Reimar! I realize I'm quite a lousy reviewer, but I'm trying to do my best! :-(
But you're always a prompt tester and more importantly, a nice guy who's easy to get along with:-)
Anway I've updated the patch according to suggestions from you and Reimar, which should be cleaner.
In this patch, long->int conversions are carried out because any of the following: 1. Assumed 32-bit access either by inline assembly or by bswap_32 macros. (e.g. variables in decode_i586.c, costab_mmx in decode_MMX.c and newhead in sr1.c) 2. Value that never exceed 32 bits. (such as variables representing sampling frequency and frame size, variables initialized to known 32-bit integers, and variables that are assigned by an 'int' and are used to pass an 'int' parameter.
I've tested the patch on IA-32 and AMD64 (thanks to Stefan's testbed!) and no regression has been found. -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hi, 2007/5/26, Zuxy Meng <zuxy.meng@gmail.com>:
Hi,
2007/5/25, Zuxy Meng <zuxy.meng@gmail.com>:
Hi,
2007/5/25, Guillaume Poirier <gpoirier@mplayerhq.hu>:
Hi,
Reimar Döffinger wrote:
Hello, On Fri, May 25, 2007 at 02:14:49PM +0800, Zuxy Meng wrote:
We've reached Part 5. This is again a big diff, replacing many 'long' to 'int', and several 'real' to 'short'. Among those, many are necessary because under LP64 environment, sizeof(long) != sizeof(int) and sizeof(long) != sizeof(float); others are optimizations simply because we really don't need such a long type, or the declaration didn't match its use (declared as float, used as short).
These are separate issues and should be fixed in different patches. Also if you changed long to int because the code assumes 32 bits, it would make more sense to change it to int32_t. Replacing long with int without thinking can make the code even slower on 64 bit systems.
[..]
This is unrelated and also completely useless since a static function that is used in only one place will be inlined anyway. In addition it is wrong since "static void make_decode_tables()" != "static void make_decode_tables(void)"
Thanks for your quality review Reimar! I realize I'm quite a lousy reviewer, but I'm trying to do my best! :-(
But you're always a prompt tester and more importantly, a nice guy who's easy to get along with:-)
Anway I've updated the patch according to suggestions from you and Reimar, which should be cleaner.
In this patch, long->int conversions are carried out because any of the following: 1. Assumed 32-bit access either by inline assembly or by bswap_32 macros. (e.g. variables in decode_i586.c, costab_mmx in decode_MMX.c and newhead in sr1.c) 2. Value that never exceed 32 bits. (such as variables representing sampling frequency and frame size, variables initialized to known 32-bit integers, and variables that are assigned by an 'int' and are used to pass an 'int' parameter.
I've tested the patch on IA-32 and AMD64 (thanks to Stefan's testbed!) and no regression has been found.
Would u help review the new patch, Reimar? -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hello, On Mon, May 28, 2007 at 09:57:14AM +0800, Zuxy Meng wrote:
2007/5/26, Zuxy Meng <zuxy.meng@gmail.com>:
I've tested the patch on IA-32 and AMD64 (thanks to Stefan's testbed!) and no regression has been found.
Would u help review the new patch, Reimar?
While I am not entirely convinced that all of it is necessary the patch seems okay to me. Except maybe that you use once *(unsigned*)hbuf and once *(uint32_t*)hbuf, I'd consider using the same in both cases. Greetings, Reimar Döffinger
Hi, 2007/5/28, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Mon, May 28, 2007 at 09:57:14AM +0800, Zuxy Meng wrote:
2007/5/26, Zuxy Meng <zuxy.meng@gmail.com>:
I've tested the patch on IA-32 and AMD64 (thanks to Stefan's testbed!) and no regression has been found.
Would u help review the new patch, Reimar?
While I am not entirely convinced that all of it is necessary the patch seems okay to me. Except maybe that you use once *(unsigned*)hbuf and once *(uint32_t*)hbuf, I'd consider using the same in both cases.
Will be corrected. Is the rest OK to apply? -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hi, 2007/5/28, Zuxy Meng <zuxy.meng@gmail.com>:
Hi,
2007/5/28, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Mon, May 28, 2007 at 09:57:14AM +0800, Zuxy Meng wrote:
2007/5/26, Zuxy Meng <zuxy.meng@gmail.com>:
I've tested the patch on IA-32 and AMD64 (thanks to Stefan's testbed!) and no regression has been found.
Would u help review the new patch, Reimar?
While I am not entirely convinced that all of it is necessary the patch seems okay to me. Except maybe that you use once *(unsigned*)hbuf and once *(uint32_t*)hbuf, I'd consider using the same in both cases.
Will be corrected. Is the rest OK to apply?
Will apply this weekend if no one disagrees. -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hi, 2007/5/30, Zuxy Meng <zuxy.meng@gmail.com>:
Hi,
2007/5/28, Zuxy Meng <zuxy.meng@gmail.com>:
Hi,
2007/5/28, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Mon, May 28, 2007 at 09:57:14AM +0800, Zuxy Meng wrote:
2007/5/26, Zuxy Meng <zuxy.meng@gmail.com>:
I've tested the patch on IA-32 and AMD64 (thanks to Stefan's testbed!) and no regression has been found.
Would u help review the new patch, Reimar?
While I am not entirely convinced that all of it is necessary the patch seems okay to me. Except maybe that you use once *(unsigned*)hbuf and once *(uint32_t*)hbuf, I'd consider using the same in both cases.
Will be corrected. Is the rest OK to apply?
Will apply this weekend if no one disagrees.
Applied. -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hi, 2007/5/25, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Fri, May 25, 2007 at 02:14:49PM +0800, Zuxy Meng wrote:
We've reached Part 5. This is again a big diff, replacing many 'long' to 'int', and several 'real' to 'short'. Among those, many are necessary because under LP64 environment, sizeof(long) != sizeof(int) and sizeof(long) != sizeof(float); others are optimizations simply because we really don't need such a long type, or the declaration didn't match its use (declared as float, used as short).
These are separate issues and should be fixed in different patches. Also if you changed long to int because the code assumes 32 bits, it would make more sense to change it to int32_t. Replacing long with int without thinking can make the code even slower on 64 bit systems.
I checked most of them, if not all, to make sure that long->int is safe. As for efficiency I don't have a box to check, but I guess 32-bit arithmetic and integer->float conversion shouldn't be slower; and on x86-64 64-bit instructions are one byte longer and is thus not recommended when unnecessary. I'll use int32_t for assumed 32-bit integers in my next patch.
[...]
Index: mp3lib/sr1.c =================================================================== --- mp3lib/sr1.c ?????? 23383?? +++ mp3lib/sr1.c ???????????? @@ -50,7 +50,6 @@ int MP3_channels=0; int MP3_bps=2;
-static long outscale = 32768; #include "tabinit.c"
#if 1
@@ -413,7 +412,7 @@ _has_mmx = 0; dct36_func = dct36;
- make_decode_tables(outscale); + make_decode_tables();
#ifdef CAN_COMPILE_X86_ASM
[...]
@@ -36,9 +36,9 @@ 64019, 65290, 66494, 67629, 68692, 69679, 70590, 71420, 72169, 72835, 73415, 73908, 74313, 74630, 74856, 74992, 75038 };
-static void make_decode_tables(long scaleval) +static void make_decode_tables() { - int i,j,k,kr,divv; + int i,j,k,kr,divv,scaleval=32768;
This is unrelated and also completely useless since a static function that is used in only one place will be inlined anyway.
What I feel uncomfortable is the file scope static variable 'outscale'. An immediate or even a constant local variable looks better to me. I had to admit it's more an aesthetic matter.
In addition it is wrong since "static void make_decode_tables()" != "static void make_decode_tables(void)"
Really? I know that () isn't equivalent to (void) in declarations but is (void) needed even in definitions? After all thanks for your kindness and patience for reviewing it! -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hello, On Fri, May 25, 2007 at 09:24:58PM +0800, Zuxy Meng wrote:
2007/5/25, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
-static void make_decode_tables(long scaleval) +static void make_decode_tables() { - int i,j,k,kr,divv; + int i,j,k,kr,divv,scaleval=32768;
This is unrelated and also completely useless since a static function that is used in only one place will be inlined anyway.
What I feel uncomfortable is the file scope static variable 'outscale'. An immediate or even a constant local variable looks better to me. I had to admit it's more an aesthetic matter.
And still unrelated ;-). And changing the static variable into a define and using that as parameter seems like a better solution to me than removing the parameter. Or if you remove the parameter, adjust the code to it. At least the first scaleval = -scaleval; is then completely useless. And if you decide to optimize that function, get rid of the useless divisions (e.g. i % 32).
In addition it is wrong since "static void make_decode_tables()" != "static void make_decode_tables(void)"
Really? I know that () isn't equivalent to (void) in declarations but is (void) needed even in definitions?
Why should it be treated different depending on whether there is a function body directly below it or not? Greetings, Reimar Döffinger
Hi, 2007/5/25, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Fri, May 25, 2007 at 09:24:58PM +0800, Zuxy Meng wrote:
2007/5/25, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
-static void make_decode_tables(long scaleval) +static void make_decode_tables() { - int i,j,k,kr,divv; + int i,j,k,kr,divv,scaleval=32768;
This is unrelated and also completely useless since a static function that is used in only one place will be inlined anyway.
What I feel uncomfortable is the file scope static variable 'outscale'. An immediate or even a constant local variable looks better to me. I had to admit it's more an aesthetic matter.
And still unrelated ;-). And changing the static variable into a define and using that as parameter seems like a better solution to me than removing the parameter.
OK. Will be in a seperate patch.
Or if you remove the parameter, adjust the code to it. At least the first scaleval = -scaleval; is then completely useless. And if you decide to optimize that function, get rid of the useless divisions (e.g. i % 32).
In addition it is wrong since "static void make_decode_tables()" != "static void make_decode_tables(void)"
Really? I know that () isn't equivalent to (void) in declarations but is (void) needed even in definitions?
Why should it be treated different depending on whether there is a function body directly below it or not?
Because the standard says so (C99 6.7.5.3.14) :-) An identifier list declares only the identifiers of the parameters of the function. An empty list in a function declarator that is part of a definition of that function specifies that the function has no parameters. The empty list in a function declarator that is not part of a definition of that function specifies that no information about the number or types of the parameters is supplied. -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
Hello, On Sat, May 26, 2007 at 12:07:32AM +0800, Zuxy Meng wrote:
2007/5/25, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Why should it be treated different depending on whether there is a function body directly below it or not?
Because the standard says so (C99 6.7.5.3.14) :-) An identifier list declares only the identifiers of the parameters of the function. An empty list in a function declarator that is part of a definition of that function specifies that the function has no parameters. The empty list in a function declarator that is not part of a definition of that function specifies that no information about the number or types of the parameters is supplied.
Maybe that really is the case for C99 (though I doubt it, and this section says nothing about when a compiler will create warning or errors). Anyway, he following code does compile with gcc, so not putting void there is a very bad idea:
static int test() { return 0; }
int main(int argc, char **argv) { return test(0); }
Greetings, Reimar Döffinger
Hi, 2007/5/26, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Sat, May 26, 2007 at 12:07:32AM +0800, Zuxy Meng wrote:
2007/5/25, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Why should it be treated different depending on whether there is a function body directly below it or not?
Because the standard says so (C99 6.7.5.3.14) :-) An identifier list declares only the identifiers of the parameters of the function. An empty list in a function declarator that is part of a definition of that function specifies that the function has no parameters. The empty list in a function declarator that is not part of a definition of that function specifies that no information about the number or types of the parameters is supplied.
Maybe that really is the case for C99 (though I doubt it, and this section says nothing about when a compiler will create warning or errors). Anyway, he following code does compile with gcc, so not putting void there is a very bad idea:
static int test() { return 0; }
int main(int argc, char **argv) { return test(0); }
Got it. Thanks! -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
participants (3)
-
Guillaume Poirier -
Reimar Döffinger -
Zuxy Meng