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

[Matt Oliver]

On 11 February 2014 02:01, Ronald S. Bultje <rsbultje at gmail.com> wrote:

> Hi,
>
> On Mon, Feb 10, 2014 at 2:53 AM, Matt Oliver <protogonoi at gmail.com> 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".
>
>
> It means I read the wrong section of the gcc inline asm manual, it's
> %k2/%k3, not %l2/%l3. Please try again with k instead of l.
>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Dont worry I think weve all been there. Anyway retried with %k2/%k3 and get
the same error. It appears icl doesnt support the k prefix. I tried with
leal and lea just in case but no luck.
For those interested error is: "error : operand 1: illegal reg in address
in asm instruction lea."