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

[Reimar Döffinger]

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.