[FFmpeg-devel] Patch: Inline asm fixes for Intel compiler on Windows

[Matt Oliver]

On 8 February 2014 20:25, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:

> Matt Oliver <protogonoi <at> gmail.com> writes:
>
> > Next round of patches
>
> They do not apply, please rebase your local checkout.
>
> > check_cc <<EOF && enable inline_asm_direct_symbol_refs
> > void foo(void){ int test; __asm__ volatile("movl test2, %eax"); }
> > #EOF
>
> Isn't the following sufficient?
> check_inline_asm inline_asm_direct_symbol_refs '"mov test, %eax\n"'
>
> > diff --git a/libavcodec/x86/cabac.h b/libavcodec/x86/cabac.h
> > index 31cb36a..f4b3a24 100644
> > --- a/libavcodec/x86/cabac.h
> > +++ b/libavcodec/x86/cabac.h
> > @@ -33,6 +33,11 @@
> >  #else
> >  #       define BROKEN_COMPILER 0
> >  #endif
> > +#if   (defined(__INTEL_COMPILER) && defined(_MSC_VER))
> > +#       define BROKEN_REGISTER_ALLOCATION 1
> > +#else
> > +#       define BROKEN_REGISTER_ALLOCATION 0
> > +#endif
>
> > diff --git a/libavcodec/x86/vp56_arith.h b/libavcodec/x86/vp56_arith.h
> > index e71dbf8..ba01412 100644
> > --- a/libavcodec/x86/vp56_arith.h
> > +++ b/libavcodec/x86/vp56_arith.h
> > @@ -24,7 +24,13 @@
> >  #ifndef AVCODEC_X86_VP56_ARITH_H
> >  #define AVCODEC_X86_VP56_ARITH_H
> >
> > +#if   (defined(__INTEL_COMPILER) && defined(_MSC_VER) && ARCH_X86_64)
> > +#       define BROKEN_REGISTER_ALLOCATION 1
> > +#else
> > +#       define BROKEN_REGISTER_ALLOCATION 0
> > +#endif
>
> Are they really different (ARCH_X86_64)?
> If not, I think you should try to avoid the duplication.
>
> Could you add the compilation error you see that makes
> patch 5 necessary?
> (Sorry if you did already.)
>
> Carl Eugen
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

> They do not apply, please rebase your local checkout.

Sorry these patches were written back when I started trying to submit these
a month or so ago so my apologies for them getting out of date. I will fix.
Is it all of the 6 patches or some in particular that I need to reed do?

> Isn't the following sufficient?

Need to also be able to declare 'int test' as other build chains may fail
due to missing declaration. check_inline_asm can be used but I though that
some things had issues with the absence of test being declared so I wanted
to make it explicit exactly what was being tested (In this case direct
references to existing variables).

> Are they really different (ARCH_X86_64)?

Intel compiler is really temperamental when it comes to inline asm. The
code works fine in 32bit but fails miserably in 64bit. The asm assembler
doesnt handle the leal instruction properly and uses the wrong register
type (i.e. 32/64bit) and then generates an error about mismatching register
types. This doesnt happen in 32 bit because all registers are 32 bit and
its fine but in 64 bit it seems to mix and match between using 32b/64b
registers for variables passed through the asm interface and then errors. I
tried finding a way around it but short of explicitly loading in the
variables and managing the registers manually there is no way to fix it
(its just an Intel bug). And I was trying to minimise code impact which is
why I didnt do that (and the generated code on other platforms may take a
slight hit). Strangely it only happens on the leal instruction and so far
only that lea instruction. It seems to be because all 3 inputs in that
instruction are interface variables and that is what is throwing the
compiler for a loop.

> Could you add the compilation error you see that makes
> patch 5 necessary?

The first is because Intel compiler doesnt like RAX being used as the
register in movzbl. So this is a 64bit issue only that generates the
following error:
\libavcodec\x86\mpegvideoenc_template.c(144): error : Unsupported
instruction form in asm instruction movzbl.

Swapping MMX and MMXEXT around is due to the following line:

#if COMPILE_TEMPLATE_MMXEXT
DECLARE_ASM_CONST(8, int16_t, mask1101[4]) = {-1,-1, 0,-1};
etc.

When MMX is done first then the compiler generates a series of errors about
missing the maskXXXX variables. Specifically:
..\libswscale\x86\yuv2rgb_template.c(321): error : identifier "mask1101" is
undefined
and many others.

It is possible to fix this by renaming the #if to instead do it "#if
COMPILE_TEMPLATE_MMX" but I instead swapped the order in case there was
some reasoning that I wasnt seeing as to why the declaration is only for
MMXEXT.