On Tue, Aug 28, 2012 at 01:54:57PM +0100, Måns Rullgård wrote:
> Diego Biurrun <di...@biurrun.de> writes:
> >
> > --- a/configure
> > +++ b/configure
> > @@ -1207,7 +1221,7 @@ HAVE_LIST="
> > yasm
> > "
> >
> > -# options emitted with CONFIG_ prefix but not available on command line
> > +# options emitted with CONFIG_/HAVE_ prefix but not available on command
> > line
> > CONFIG_EXTRA="
>
> That comment change makes no sense.
Umm, I add HAVE_EXTRA directly below CONFIG_EXTRA ...
> > @@ -1240,6 +1254,14 @@ CONFIG_EXTRA="
> >
> > +ARCH_EXT_LIST_X86_INLINE=$(add_prefix inline_ $ARCH_EXT_LIST_X86)
> > +ARCH_EXT_LIST_X86_YASM=$(add_prefix yasm_ $ARCH_EXT_LIST_X86)
>
> What, no vertical alignment?
You know very well that it's overrated ;)
It's kinda hard in shell as the syntax disallows spaces before = in assignments.
> > @@ -1318,15 +1340,47 @@ ppc4xx_deps="ppc"
> > vis_deps="sparc"
> >
> > x86_64_suggest="cmov fast_cmov"
> > +
> > amd3dnow_deps="mmx"
> > amd3dnowext_deps="amd3dnow"
> > mmx_deps="x86"
> > mmxext_deps="mmx"
> > -sse_deps="mmx"
> > -ssse3_deps="sse"
> > -avx_deps="ssse3"
> > +sse_deps="mmxext"
> > +sse2_deps="sse"
> > +sse3_deps="sse2"
> > +ssse3_deps="sse3"
> > +sse4_deps="ssse3"
> > +sse42_deps="sse4"
> > +avx_deps="sse42"
> > fma4_deps="avx"
> >
> > +inline_amd3dnow_deps="inline_mmx amd3dnow"
> > +inline_amd3dnowext_deps="inline_amd3dnow amd3dnowext"
> > +inline_mmx_deps="inline_asm mmx"
> > +inline_mmxext_deps="inline_mmx mmxext"
> > +inline_sse_deps="inline_mmxext sse"
> > +inline_sse2_deps="inline_sse sse2"
> > +inline_sse3_deps="inline_sse2 sse3"
> > +inline_ssse3_deps="inline_sse3 ssse3"
> > +inline_sse4_deps="inline_ssse3 sse4"
> > +inline_sse42_deps="inline_sse4 sse42"
> > +inline_avx_deps="inline_sse42 avx"
> > +inline_fma4_deps="inline_avx fma4"
> > +
> > +yasm_amd3dnow_deps="yasm_mmx amd3dnow"
> > +yasm_amd3dnowext_deps="yasm_amd3dnow amd3dnowext"
> > +yasm_mmx_deps="yasm mmx"
> > +yasm_mmxext_deps="yasm_mmx mmxext"
> > +yasm_sse_deps="yasm_mmxext sse"
> > +yasm_sse2_deps="yasm_sse sse2"
> > +yasm_sse3_deps="yasm_sse2 sse3"
> > +yasm_ssse3_deps="yasm_sse3 ssse3"
> > +yasm_sse4_deps="yasm_ssse3 sse4"
> > +yasm_sse42_deps="yasm_sse4 sse42"
> > +yasm_avx_deps="yasm_sse42 avx"
> > +yasm_fma4_deps="yasm_avx fma4"
>
> for ext in $ARCH_EXT_LIST_X86; do
> eval dep=\$${ext}_deps
> eval inline_${ext}_deps='"inline_$dep $ext"'
> eval yasm_${ext}_deps='"yasm_$dep $ext"'
> done
>
> Or something like that.
I'll try that or something similar in the next iteration.
> > @@ -1863,7 +1917,10 @@ for n in $COMPONENT_LIST; do
> > eval ${n}_if_any="\$$v"
> > done
> >
> > -enable $ARCH_EXT_LIST $ALL_TESTS
> > +enable $ARCH_EXT_LIST \
> > + $ARCH_EXT_LIST_X86_INLINE \
> > + $ARCH_EXT_LIST_X86_YASM \
> > + $ALL_TESTS \
>
> This will get out of hand quickly if we want to do something similar for
> other architectures. Could the same thing be achieved by in the loop I
> propose above adding this line:
>
> eval ${ext}_suggest='"inline_$ext yasm_$ext"'
>
> For future expansion, we might also want to use something more generic
> than yasm in these variable names.
We have already talked about naming surrounding assemblers/assembly
without tangible results. The best I can come up with are
"inline_asm" and "standalone_asm", do you have a better suggestion?
> > @@ -3352,14 +3409,15 @@ fi
> >
> > +enabled asm || { arch=c; disable $ARCH_LIST $ARCH_EXT_LIST; }
> > check_deps $CONFIG_LIST \
> > $CONFIG_EXTRA \
> > $HAVE_LIST \
> > + $HAVE_EXTRA \
> > $ALL_COMPONENTS \
> > $ALL_TESTS \
> >
> > -enabled asm || { arch=c; disable $ARCH_LIST $ARCH_EXT_LIST; }
> > -
> > ! enabled_any memalign posix_memalign aligned_malloc &&
> > enabled_any $need_memalign && enable memalign_hack
>
> Moving that line is probably OK.
I thought I needed it to ensure correct functioning of --disable-asm, but
on second thought I think I can drop it.
> > --- a/libavfilter/x86/gradfun.c
> > +++ b/libavfilter/x86/gradfun.c
> > @@ -24,12 +24,10 @@
> > #include "libavutil/x86/asm.h"
> > #include "libavfilter/gradfun.h"
> >
> > -#if HAVE_INLINE_ASM
>
> Why do you remove that line?
I'm replacing HAVE_FOO below with HAVE_INLINE_FOO, which already
ensures that HAVE_INLINE_ASM is set.
> > @@ -165,26 +163,16 @@ static void gradfun_blur_line_sse2(uint16_t *dc,
> > uint16_t *buf, uint16_t *buf1,
> >
> > av_cold void ff_gradfun_init_x86(GradFunContext *gf)
> > {
> > int cpu_flags = av_get_cpu_flags();
> >
> > -#if HAVE_INLINE_ASM
> > -#if HAVE_MMXEXT
> > - if (cpu_flags & AV_CPU_FLAG_MMXEXT)
> > + if (cpu_flags & AV_CPU_FLAG_MMXEXT && HAVE_INLINE_MMXEXT)
> > gf->filter_line = gradfun_filter_line_mmx2;
>
> This will fail if HAVE_INLINE_MMXEXT == 0.
It's already gone from the version I have locally.
> > --- a/libavutil/x86/float_dsp_init.c
> > +++ b/libavutil/x86/float_dsp_init.c
> > @@ -33,16 +33,14 @@ extern void ff_vector_fmac_scalar_avx(float *dst, const
> > float *src, float mul,
> >
> > void ff_float_dsp_init_x86(AVFloatDSPContext *fdsp)
> > {
> > -#if HAVE_YASM
> > int mm_flags = av_get_cpu_flags();
> >
> > - if (mm_flags & AV_CPU_FLAG_SSE && HAVE_SSE) {
> > + if (mm_flags & AV_CPU_FLAG_SSE && HAVE_YASM_SSE) {
> > fdsp->vector_fmul = ff_vector_fmul_sse;
> > fdsp->vector_fmac_scalar = ff_vector_fmac_scalar_sse;
> > }
> > - if (mm_flags & AV_CPU_FLAG_AVX && HAVE_AVX) {
> > + if (mm_flags & AV_CPU_FLAG_AVX && HAVE_YASM_AVX) {
> > fdsp->vector_fmul = ff_vector_fmul_avx;
> > fdsp->vector_fmac_scalar = ff_vector_fmac_scalar_avx;
> > }
> > -#endif
> > }
>
> Why do you remove the #if HAVE_YASM? I imagine it's there for a reason.
Same reasoning here, I'm replacing it with something more specific.
Note that the code still compiles with --disable-yasm.
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
