[MPlayer-dev-eng] [PATCH] Make mp3lib SIMD optimizations work on AMD64, Part 5

[Zuxy Meng]

Hi,

2007/5/25, Reimar Döffinger <Reimar.Doeffinger at 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