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

[Matt Oliver]

On 3 February 2014 18:45, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

>
>
> On 03.02.2014, at 08:13, Matt Oliver <protogonoi at gmail.com> wrote:
>
> > 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.
>
> Looks good to me.
>
> > Patch 2: This is the inline asm patch similar to what I have previously
> > submitted. From previous suggestions the libmpcodec stuff has been
>
> Could you please split it by issue?
> As said you change to use cdq _will_ break things, so I will object as
> long as it's there.
> If you split it by "type of change" it can be applied step by step.
> Also the BROKEN_COMPILER define is already bad naming, it would be good if
> you can think of a better name than PARTIAL_BROKEN_COMPILER
>
>
> > 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 change is also problematic I suspect, at least I remember some issues
> with gcc generating labels itself that ended up with the same name as our
> inline asm ones, which breaks things in a very bad way.
> However I do not remember which names were safe and which weren't, so it's
> possible that your change even fixes things for gcc...
>
>
> > Patch 3: This is the inline asm changes specific for libmpcodecs. If you
> > want me to submit this separately then let me know.
>
> Well, having the DECLARE_ALIGNED changes separately would be nice since I
> think that part can be applied without further review.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

I can split the libmpcodecs patch into 2 if needed.

Although I must have missed the issue with cdq being raised (or forgot).
Given cltd will not work at all in Intel, both work for gcc and apparently
cdq does not work with other compilers what is the recommend way to get
around that? A define would be the obvious one but a little ugly. Any
suggestions?

Also if BROKEN_COMPILER is bad naming what would you suggest? Unfortunately
its one of those situations were the compiler is behaving badly so I
figured the name was appropriate.