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

Mon Feb 3 08:13:31 CET 2014

On 27 January 2014 12:07, Matt Oliver <protogonoi at gmail.com> wrote:

> > 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
>

OK So I have split the patch up into 4 smaller patches. Ill list them here
to get feedback first but if people want me to create a new email thread
for each one then let me know and i will.

Patch 1: Removed 2 minor fixes that are independent of inline asm and are
required just to get intel compiler to work in general. These are found in
this patch.

Patch 2: This is the inline asm patch similar to what I have previously
submitted. From previous suggestions the libmpcodec stuff has been
separated into a separate patch. Ive also cleaned up most of the
"defined(__INTEL_COMPILER) && defined(_MSC_VER)" from all around the place.
I added 2 defines; these are for HAVE_INLINE_ASM_NONLOCAL_LABELS which is
used to determine if a compiler supports inline asm nonlocal label names
(where a local name is just a number and is defined in local scope e.g. 1:
). This define replaces a previous specific intel compiler check.

Another 2 locations check for intel and then create a BROKEN_COMPILER
define. This is exactly what was previously done in x86/cabac.h for the
clang compiler so I did exactly the same thing. The difference is that the
clang broken compiler skips all 3 functions in cabac.h whereas the Intel
compiler only breaks on the first (get_cabac_inline_x86). This prevented me
from just reusing the existing clang BROKEN_COMPILER. Its quite possible
that clang only brakes on the first function aswell but no one tested the
whole thing. So if this is the case and the last 2
functions (get_cabac_bypass_sign_x86, get_cabac_bypass_x86) actually work
under clang then the defines can be merged but otherwise theyll need to
remain separate. But atleast now it cleans things up a bit and doesnt add
anything much that wasnt already done.

Patch 3: This is the inline asm changes specific for libmpcodecs. If you
want me to submit this separately then let me know.

Patch 4: Updates to configure to support generation of the new define
HAVE_INLINE_ASM_NONLOCAL_LABELS and
previous HAVE_INLINE_ASM_DIRECT_SYMBOL_REFS. Also no longer disables inline
asm when using icl. The configure changes are in a separate patch in case
they affect other build chains. I have tested with gcc and all works but
havnt got access to a clang machine.

Also im having unrelated icl issues with configure anyway. I must admit
that all my testing of the actual inline asm code was performed directly
within visual studio as I wrote a program (admittedly a rather hacky one)
that reads in the existing configure/make scripts and any applicable
command line arguments and dynamically generates an appropriate Visual
Studio project file. So I can test all the intel/msvc specific code
directly from Visual Studio without even needing an msys environment.

Feedback welcome.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Fix-compilation-with-msvc-icl-due-to-missing-header-.patch
Type: application/octet-stream
Size: 1238 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140203/c6c14102/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Fix-inline-asm-compilation-with-Intel-compiler-on-Wi.patch
Type: application/octet-stream
Size: 41669 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140203/c6c14102/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Fix-libmpcodecs-inline-asm-compilation-with-Intel-co.patch
Type: application/octet-stream
Size: 12552 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140203/c6c14102/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Update-configure-script-to-support-Intel-inline-asm.patch
Type: application/octet-stream
Size: 1733 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140203/c6c14102/attachment-0003.obj>