[FFmpeg-devel] [PATCH] yadif sse2/ssse3

[Ronald S. Bultje]

Hi Baptiste,

On Sat, Nov 20, 2010 at 9:32 PM, Baptiste Coudurier
<baptiste.coudurier at gmail.com> wrote:
> Hi guys,

Thanks for doing this. A few generic comments (didn't look at the code
itself yet, I'm sorry):

> +#if HAVE_SSE

I consider the HAVE_xxx namespace reserved for configure:

bash-3.2$ grep HAVE_MMX config.h
#define HAVE_MMX 1
#define HAVE_MMX2 1
bash-3.2$ grep HAVE_SSE config.h
#define HAVE_SSE 1
bash-3.2$

So I'd prefer if you'd use something else for the template naming (I
don't really know what, maybe peek at the template files in
libavcodec/x86/?). Same for HAVE_SSSE3 and so on.

> +#define MM "%%xmm"
> +#define MOV  "movq"
> +#define MOVQ "movdqa"
> +#define MOVQU "movdqu"
> +#define STEP 8

This is a good idea - our external asm code already does this. Maybe
(I'm not forcing, just kindly suggesting) it's a good idea to use the
same names for the same things in both internal or external asm. MM is
called m in .asm files, so you could use M##<num>. MOV is called movh
(H=half, either movd or movq), whereas MOVQ is called mova (mov full
aligned, either movq or movdqa), and MOVQU is movu (mov full
unaligned, either movq or movdqu). STEP*2 is called mmsize (16 or 8).

+#define PSRL1(reg) "psrldq $1, "reg" \n\t"
+#define PSRL2(reg) "psrldq $2, "reg" \n\t"
+#define PSHUF(src,dst) "movdqa "dst", "src" \n\t"\
+                       "psrldq $2, "src"     \n\t"

Could you just define the suffix here (dq vs q) instead of having full
defines for the whole thing?

Ronald