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

[Ronald S. Bultje]

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