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

Matt Oliver protogonoi at gmail.com
Mon Jan 27 02:07:59 CET 2014


> Matt Oliver <protogonoi <at> gmail.com> writes:
>
> > On 5 January 2014 13:27, Matt Oliver <protogonoi <at> gmail.com> wrote:
>
> > +// Determine if compiler support direct symbol references in inline asm
> > +#if defined(__INTEL_COMPILER) && defined(_MSC_VER)
> > +#   define HAVE_DIRECT_SYMBOL_REF 0
> > +#else
> > +#   define HAVE_DIRECT_SYMBOL_REF 1
> > +#endif
>
> You should test this in configure.
> And please use this define all over the patch instead
> of "defined intel && defined msvc".
>
> I would suggest you postpone the changes to libavfilter/libmpcodecs,
> test with --disable-filter=mp (or disable-gpl).
> Once your initial patch hits git, send a patch for libmpcodecs
> either here or to mplayer-dev-eng and we will commit it.
>
> > +//MMX versions
>
> Is this move needed / related?
> If not, please remove it, such unintended cosmetic
> changes should not be committed to the repository.
>
> >              "r" (dest), "m" (dstW_reg), "m"(uv_off) \
> > +
> > +#define YSCALEYUV2PACKEDX_END2                    \
>
> I don't know much about it, but doesn't this empty line
> hurt?
>
> > +//Declared in swscale_mmx.c (and referenced from rgb2rgb_template.c)
> > +DECLARE_ALIGNED(8, extern const uint64_t, ff_bgr2YOffset);
>
> Is there no common header?
>
> > +#ifndef M_PI_2
> > +#define M_PI_2         1.57079632679489661923  /* pi/2 */
> > +#endif
>
> Is this related? What does it fix?
>
> > +#include "config.h"
> > +#if HAVE_SYS_TIME_H
> >  #include <sys/time.h>
> > +#endif
> >  #include <time.h>
> >
> > -#include "config.h"
>
> If this is needed on some platform, please provide an
> independent patch with a short explanation.
>
> Carl Eugen


> You should test this in configure.
> And please use this define all over the patch instead of "defined intel
&& defined msvc".

Currently only Intel on Windows sets the HAVE_DIRECT_SYMBOL_REF to 0 so it
would be possible to use that define instead however the other uses of
"defined intel && defined msvc" are there because those sections even when
changed to not using direct symbol references will still not compile. ASM
in Intel compiler is extremely sensitive and some sections of code that
even when using completely supported features will often not compile. So
these sections are skipped using the "defined intel && defined msvc" not
because of an issue with direct symbol references but for an issue that
only affects the intel compiler and only on Windows. So i didnt want there
to be any confusion should at some point down the line another compiler
fails the "have direct symbol" check. I admit that the defines are rather
ugly but are technically the only way around it.

The only exception to this is in mlpdsp.c where the "defined intel &&
defined msvc" is because Intel does not support custom labels in asm so
that could be replaced with a HAVE_NAMED_SYMBOLS if you would prefer.

> I would suggest you postpone the changes to libavfilter/libmpcodecs,
> test with --disable-filter=mp (or disable-gpl).

I can do that if thats what people want.

> > +//MMX versions
>
> Is this move needed / related?

Yes, The order that the files are included affects the end result.
Compilation fails with the previous order but will work when rearranged
(sounds weird I know).

> I don't know much about it, but doesn't this empty line
> hurt?

Your right, probably should be removed.

> Is there no common header?

Nope, these are defined in swscale.c hence the need for the additional
externs.

> Is this related? What does it fix?

The M_PI_2 and #if HAVE_SYS_TIME_H are somewhat related. They are not
necessary for fixing inline asm for Intel but they are necessary in that
without them the Intel compiler will not compile anything (inline or not).
So I was thinking of this patch as a fix for Intel compilation, but if you
want it to be fix for intel asm compilation only then I can remove it into
another patch. But without it then this patch wont compile anyway so thats
why I included it. However if you want them separated then let me know.

Matt


More information about the ffmpeg-devel mailing list