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

Michael Niedermayer michaelni at gmx.at
Mon Feb 10 17:33:36 CET 2014


On Mon, Feb 10, 2014 at 06:53:23PM +1100, Matt Oliver wrote:
> On 10 February 2014 09:21, Reimar Döffinger <Reimar.Doeffinger at gmx.de>wrote:
> 
> > On Thu, Feb 06, 2014 at 12:35:13PM +1100, Matt Oliver wrote:
> >
> > > On 6 February 2014 05:45, Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> > wrote:
> > >
> > > > On Wed, Feb 05, 2014 at 11:41:38AM +1100, Matt Oliver wrote:
> > > > > Cheers for the feedback. Ill split up the patches to make things
> > easier.
> > > > To
> > > > > start off I have split the libmpcodec patches into some smaller
> > easily
> > > > > digestible chunks.
> > > > >
> > > > > Patch 1: Changes a couple of inline asm labels for using named
> > labels to
> > > > > standard numbered labels. This has no affect but to allow for
> > compilation
> > > > > with compilers that dont support named labels.
> > > >
> > > > If you use numbered labels I believe you need to append the jump
> > > > direction.
> > > > I.e.
> > > > jnz 1b
> > > > instead of
> > > > jnz 1
> > > > if you mean the 1: label that comes before.
> > > > At least that is what I see in e.g. libavcodec/x86/snowdsp.c and many
> > > > more files.
> > > >
> > > > > > Well, having the DECLARE_ALIGNED changes separately would be nice
> > > > since I
> > > > > think that part can be applied without further review.
> > > > >
> > > > > Patch 2: Is just the declare aligned fixes. This uses the existing
> > cross
> > > > > platform specific declare aligned macro instead of the gcc only
> > > > attribute.
> > > > > Seperated as Reimar suggested.
> > > >
> > > > You seem to have lost the very first "static". Otherwise it looks ok to
> > > > me.
> > > >
> > > > > Patch 3: Removes unnecessary commas from some inline asm. Leaving the
> > > > > commas in with nothing following them causes icl compilation errors.
> > > > > removing them has no impact on anything else as ive seen this used
> > else
> > > > > where in ffmpeg so this should have no impact.
> > > >
> > > > Looks good to me as well.
> > > > If nobody has any other comments I will try to get it applied to
> > MPlayer
> > > > soon to try to keep the codebases in sync (or if I'm too slow
> > > > anyone with commit access there is of course free to apply them).
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel at ffmpeg.org
> > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >
> > >
> > > > If you use numbered labels I believe you need to append the jump
> > > > direction.
> > >
> > > You only have to do this if you have multiple labels with the same name.
> > In
> > > this case the forward/backward identifiers are so the assembler knows
> > which
> > > label you are referring to (closest in front or closest behind). Since
> > the
> > > labels in the patch are all unique then there is only one so we dont need
> > > to specify which direction to find it.
> >
> > I don't trust that this is always true, and I see no reason to do things
> > different than all other places I could find.
> > So I changed that.
> >
> > > > You seem to have lost the very first "static". Otherwise it looks ok to
> > > > me.
> > >
> > > That I did, fixed in updated patch attached. I changed to using the
> > > predefined ASM_CONSTANT macro that auto includes static const.
> >
> > You changed one uint8_t to int8_t, I fixed that before applying.
> > Otherwise all patches have been applied to MPlayer's libmpcodecs.
> > I have _not_ synced the FFmpeg code, if someone can easily do that
> > please go ahead.
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> 
> Please provide the relevant (!!) part of config.log
> 
> 
> Sorry Carl, I rescind my previous statement. Gcc works fine it was my MinGW
> that was failing and after checking the logs a second time I realised it
> was actually due to a random write permission error in creating the temp
> file that has started cropping up since I moved to Win8.1. So it could be
> changed if thats what people want. In not 100% about the change myself as
> if Intel do update there compiler with direct symbol references then due to
> the way they resolve references currently without the 'test' declaration it
> would still fail. This is a future proof thing so Im happy to concede to
> whatever the group consensus is.
> 
> 
> > I'm not going to argue against this being an icl bug, but I'm surprised you
> > can't come up with a concept that works. Doesn't something like "leal (%l2,
> > %l3), %l3" work? Also, which one does it convert to 64bit?
> 
> 
> Ive tried everything I can think of so Im certainly open to suggestions.
> Ive tried every combination of constraints I could think off (although
> Intel doesnt support all of them) and just could not get it to work. As to
> which operand register is being set to either 32bit/64bit im not sure as
> the error message doesnt specify so the only way would be to try and find
> the line in the compiler intermediary output (which im understandably in no
> hurry to attempt). All i can ascertain from the error message is one of
> %2/%3 is 32bit and the other is 64bit. As to why the difference I have no
> idea as they both have the same constraint and are both uint. I tried your
> suggestion about prefixing operands with 'l' (although im not familiar with
> this technique) a direct substitution resulted in the error: "Substitution
> directive specifies non-existent operand in asm instruction". Although as I
> said im not familiar with that technique (im guessing the prefix forces it
> to use long sized registers?) i may be missing something. But nothing ive
> tried works, even trying to change the leal to just lea or something
> generates errors (illegal reg in address for those who are interested). So
> I ran out of ideas. Basically everything else ive tried generates the
> "Unsupported instruction form" error. Intel just does not like lea
> instructions where the input are not explicit registers. Thats the only way
> I know of to fix it is to move say %2 into an explicit register and then
> use that but that would potentially negatively affect other buildchains. If
> there is a fix I would love to know it.
> 
> You changed one uint8_t to int8_t, I fixed that before applying.
> > Otherwise all patches have been applied to MPlayer's libmpcodecs.
> > I have _not_ synced the FFmpeg code, if someone can easily do that
> > please go ahead.
> >
> 
> Must have missed that when i updated, Thanks Reimar for picking it up and
> fixing it.
> 
> Also some recent changes in master have added some new asm in the file
> dca.h. Attached is a standalone patch that fixes that for intel
> compatibility (I didnt add it to the previous patch so that for those who
> have already looked over and ok'ed the current version dont have to recheck
> them).

>  dca.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> dfdf0508b391e5e0f7f68344b6798802289af7ee  7-6-Additional-icl-inline-asm-fix.patch
> From 7929bc1ae54be0f79b4407b8916d5e42d69f4fcf Mon Sep 17 00:00:00 2001
> From: Matt Oliver <protogonoi at gmail.com>
> Date: Mon, 10 Feb 2014 18:51:48 +1100
> Subject: [PATCH] [7/6] Additional icl inline asm fix.
> 
> ---
>  libavcodec/x86/dca.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/x86/dca.h b/libavcodec/x86/dca.h
> index 6415129..cefac59 100644
> --- a/libavcodec/x86/dca.h
> +++ b/libavcodec/x86/dca.h
> @@ -31,7 +31,7 @@ static inline void int8x8_fmul_int32(av_unused DCADSPContext *dsp,
>  {
>      DECLARE_ALIGNED(16, static const uint32_t, inverse16) = 0x3D800000;
>      __asm__ volatile (
> -        "cvtsi2ss        %2, %%xmm0 \n\t"
> +        "cvtsi2ssl       %2, %%xmm0 \n\t"
>          "mulss           %3, %%xmm0 \n\t"
>          "movq          (%1), %%xmm1 \n\t"
>          "punpcklbw   %%xmm1, %%xmm1 \n\t"

In file included from ffmpeg/libavcodec/dcadec.c:1:
In file included from ffmpeg/libavcodec/dcadec.c:53:
ffmpeg/libavcodec/x86/dca.h:34:9: error: invalid instruction mnemonic 'cvtsi2ssl'
        "cvtsi2ssl       %2, %%xmm0 \n\t"
        ^
<inline asm>:1:2: note: instantiated into assembly here
        cvtsi2ssl       -44(%rbp), %xmm0
        ^~~~~~~~~

btw iam starting to think that the issue should be fixed on the
icl side or some wraper script be written to do all these translations

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140210/7b3f3b7e/attachment.asc>


More information about the ffmpeg-devel mailing list