[FFmpeg-devel] [PATCH] split-radix FFT

[Måns Rullgård]

Loren Merritt <lorenm at u.washington.edu> writes:

> On Tue, 5 Aug 2008, M?ns Rullg?rd wrote:
>> Loren Merritt <lorenm at u.washington.edu> writes:
>>
>>> @@ -829,6 +838,7 @@ mmx2_deps="x86 mmx"
>>>  neon_deps="armv4l"
>>>  ssse3_deps="x86"
>>>  vis_deps="sparc"
>>> +yasm_deps="x86 mmx"
>>
>> What is the purpose of this?  Yasm is only used on x86, and it could
>> be used for non-mmx code.  (Side-note: should we separate mmx/sse/sse2?)
>> Either way, that line is in the wrong place; yasm is not a CPU feature.
>
> I added that because I replaced a HAVE_MMX with a HAVE_YASM, but I
> guess it's not needed.

Yes, since SIMD support is detected at runtime, it is enough check
whether the code will build.

>> What is the correct value for objformat on win64?
>
> "win64"
>
> Hmm, specifying arch separately is an alternative to including bitness
> in the objformat, so I can simplify the case too.
>
>>> @@ -1534,6 +1551,25 @@ EOF
>>>      enabled mmx2  && check_asm mmx2  '"movss %xmm0, %xmm0"'
>>>
>>>      check_asm bswap '"bswap %%eax" ::: "%eax"'
>>> +
>>> +    enable yasm
>>> +    if enabled x86_64; then
>>> +        YASMFLAGS="-DARCH_X86_64"
>>> +        enabled shared && append YASMFLAGS "-DPIC"
>>> +        case "$objformat" in
>>> +            elf) objformat="elf64";;
>>> +            macho) objformat="macho64";;
>>> +            win32) disable yasm;;
>>
>> As I said, we shouldn't break win64 (even if I don't care personally).
>
> With this line, nothing breaks, only a speed regression in mdct

I call that a form of breakage.

> codecs. Without this line, it would break until someone ports the
> calling convention macros.

Ramiro apparently has a win64 system.  Maybe he can help.

> OK, I'll try. Here's a completely untested attempt at win64.
> I have intentionally omitted the complex stack frame mandated by the
> win64 abi. I'm not entirely sure what effect this omission has, but
> Squid_80 tells me that all it should do is confuse the exception
> handler in the event of a crash.

Why, oh why, do they always make things so complicated in windows?

>> Hmm, maybe YASMFLAGS="-DARCH_$(toupper $arch) -f $objformat"
>
> done
>
>>> +    check_yasm "pabsw xmm0, xmm0" || disable yasm
>>
>> Come to think of it, isn't enough to check_cmd yasm --version?  Either
>> it's there, or it's not.
>
> Either it's there, or it's not, or it's an obsolete version. I don't
> want more variants of HAVE_SSSE3 than necessary. And if not ssse3, I'd
> still have to check if it's too old to support x86_64.

Good point.  Keep the check.  I don't know which instructions belong
to which category of mmx, mmx2 mmxext, sse, sse2, sse3, ssse3, sse4,
sse4.5, ssssse5, etc. (and I have no intention of learning it), so it
wasn't obvious to me what was being tested.

> @@ -1534,6 +1550,15 @@ EOF
>      enabled mmx2  && check_asm mmx2  '"movss %xmm0, %xmm0"'
>
>      check_asm bswap '"bswap %%eax" ::: "%eax"'
> +
> +    YASMFLAGS="-f $objformat -DARCH_$(toupper $arch)"
> +    enabled x86_64 && append YASMFLAGS "-m amd64"
> +    enabled_all x86_64 shared && append YASMFLAGS "-DPIC"
> +    case "$objformat" in
> +        elf*) enabled debug && append YASMFLAGS "-g dwarf2";;
> +        *) append YASMFLAGS "-DPREFIX";;
> +    esac

This bit could use be written a bit prettier.  I'm thinking something
like this:

    enabled     x86_64        && append YASMFLAGS "-m amd64"
    enabled_all x86_64 shared && append YASMFLAGS "-DPIC"
    case "$objformat" in
        elf*) enabled debug && append YASMFLAGS "-g dwarf2" ;;
        *)                     append YASMFLAGS "-DPREFIX"  ;;
    esac

The rest is OK.

If nobody steps up to test this on win64, I suggest we simply check it
in, and deal with any bugreports that might result.

-- 
M?ns Rullg?rd
mans at mansr.com