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

[Matt Oliver]

On 5 February 2014 06:37, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> On Mon, Feb 03, 2014 at 09:16:47PM +1100, Matt Oliver wrote:
> > 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:
> > > > 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.
> > >
> >
> > I can split the libmpcodecs patch into 2 if needed.
>
> "Needed" is an exaggeration, but the larger the patch, the fewer
> people will have time to review, the more review comments you'll
> get all over the place, it makes debugging the changes later harder etc.
> Splitting is a simple way to reduce the amount of code changes
> you have locally.
>
> > Although I must have missed the issue with cdq being raised (or forgot).
>
> Mentioned it in both of my earlier partial reviews.
>
> > 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?
>
> Define doesn't sound so bad, but to be honest this change is in the
> middle of a huge number of other changes so I have 0 overview and
> feel unable to really get an idea of what would be appropriate.
> Which is why I suggest splitting patches: reduce the complexity
> of the problem by solving it step by step instead of all in one go.
> With the trivial stuff first since that is unlikely to need changes
> and should go in quickly.
>
> > 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.
>
> Well, the "problem" is that a compiler can be broken in hundreds of
> ways, something more specific would seem helpful.
> Since I don't know/haven't checked what exactly it is about I don't
> have any suggestion, but in general the question to ask is:
> If you look at the code in 5 years, what name might help you understand
> what goes on?
> I'm fairly certain that "BROKEN_COMPILER" will be almost as helpful a
> name for understanding as just calling it "FJSNVGFRVB".
> But yes, it's not a big deal, just a suggestion to think about.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

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.

> 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.

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.

There is a fourth patch for libmpcodecs but it requires the asm.h changes
from the previous inline asm patch so I will include that one later once
the main ffmpeg patch has been cleaned up and excepted. The above 3 patches
are completely stand alone.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1-4-libmpcodecs-fix-inline-asm-labels.patch
Type: application/octet-stream
Size: 1581 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140205/1a30d64c/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 2-4-Fix-libmpcodecs-data-align-for-cross-platform-co.patch
Type: application/octet-stream
Size: 3921 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140205/1a30d64c/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 3-4-Fix-libmpcodecs-inline-asm-on-ICL.patch
Type: application/octet-stream
Size: 5234 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140205/1a30d64c/attachment-0002.obj>