[MPlayer-dev-eng] [PATCH] x86_64 mmx/sse/3dnow optimisation support

[D Richard Felker III]

On Sun, Oct 17, 2004 at 11:24:22PM +0200, Aurelien Jacobs wrote:
> On Sun, 17 Oct 2004 11:30:19 -0400
> D Richard Felker III <dalias at aerifal.cx> wrote:
> 
> > On Sun, Oct 17, 2004 at 04:41:28PM +0200, Aurelien Jacobs wrote:
> > > > just don't tell pullup that mmx/etc is supported if you're on x86-64
> > > > and maybe sometime i'll come up with a better solution. (note that
> > > > pullup.c did not already include cpudetect.h, for good reason!)
> > > 
> > > Ok.
> > > I see 2 solutions :
> > >  - I could replace the inclue of cpudetect.h by defines of REG_a, etc..
> > >    which would still keep mmx supported on x86_64 without mplayer
> > >    dependencie.
> > >  - Else, I'll need to replace the #ifdef HAVE_MMX by a 
> > >    #if defined(HAVE_MXX) && !defined(ARCH_X86_64)
> > > 
> > > Which one do you prefer ? (I guess the second, but keeping x86_64 mmx
> > > support may be nice)
> > 
> > approximately the second. but put a separate #ifndef ARCH_X86_64 (or
> > preferably #ifdef ARCH_X86, if it's not defined for x86-64) on the
> > outside around the other #ifdef, since it's possible i'll add 3dnow or
> > other optimized functions later.
> 
> Ok. Done.
> 
> > > > otherwise, i see lots of unnecessary changes from int->long in vf_*.c.
> > > > please comment on why their necessary. i agree x86-64 support is
> > > > useful, but i don't like excessive cosmetic patches especially when
> > > > they reduce readability excessively like this.
> > > 
> > > I don't like cosmetic too !
> > > If I added so much int->long that's because they were needed.
> > > 
> > > Here is an exemple :
> > > 	    "movq (%0,%2,2), %%mm1\n\t"
> > > ....
> > > -	    : "r"(as), "r"(bs), "m" (ones), "0"(a), "1"(b), "X"(*a), "X"(*b) \
> > > +	    : "r"((long)as), "r"((long)bs), "m" (ones), "0"(a), "1"(b), "X"(*a),
> > > 
> > > Here, %0 is used for addressing, so it need to be a 64 bits register.
> > > as and bs are ints, so they will expand to 32 bits register and this
> > > will prevent compilation.
> > 
> > casting to long like this is fine. but there were lots of local
> > variables where you changed the types.
> 
> I tried not to modify variable types too much, but in some cases it was
> better, or even necessary. Most of the local types changes looks like
> this :
> -	int crap1, crap2;
> +	long crap1, crap2;
> ...
>  			: "=S"(crap1), "=D"(crap2)
> 
> So those variables are in fact the output values of some asm come.
> The asm output 64 bits values, so it need a 64 bits variable for
> storage. Anyway, gcc will complain if you try to use a cast as a
> lvalue.
> 
> > there were also functions where
> > you changed the types of function arguments which i find really bad.
> 
> Right. I also find this very bad !
> In fact I tried to avoid this as much as possible. But when modifing
> vf_ilpack.c, I found it would be far cleaner to modify parameters type,
> and as those functions were statics and were used only once, I thought
> it would not be a big problem.
> At the end I used a totally different way to port those functions to
> x86_64, but I forgot to check if the functions type modification was
> still necessary. The good news is that it's not !
> So I corrected this.
> vf_ilpack.c was the only file where I modified some parameters type.
> 
> Aurel

ok, this all sounds much better. i haven't read the patch yet but from
what you've said i think it's good so far.

rich