[FFmpeg-devel] [PATCH 2/2] x86: convert DNxHDenc inline asm to yasm

[Ronald S. Bultje]

Hi,

On Wed, Mar 26, 2014 at 10:06 PM, Timothy Gu <timothygu99 at gmail.com> wrote:

> +INIT_XMM sse2
> +cglobal get_pixels_8x4_sym, 3,3,5, block, pixels, linesize
> +    pxor      m4,       m4
> +    movq      m0,       [pixelsq]
> +    add       pixelsq,  linesizeq
> +    movq      m1,       [pixelsq]
> +    movq      m2,       [pixelsq+linesizeq]
> +    movq      m3,       [pixelsq+linesizeq*2]
> +    punpcklbw m0,       m4
> +    punpcklbw m1,       m4
> +    punpcklbw m2,       m4
> +    punpcklbw m3,       m4
> +    mova  [blockq    ], m0
> +    mova  [blockq+16 ], m1
> +    mova  [blockq+32 ], m2
> +    mova  [blockq+48 ], m3
> +    mova  [blockq+64 ], m3
> +    mova  [blockq+80 ], m2
> +    mova  [blockq+96 ], m1
> +    mova  [blockq+112], m0
> +    RET
>

Patch OK.

I do have a comment about the indenting. I don't want to hold up patches
for indenting, plus it might be intentional (in which case it's fine). But
it may also be unintentional, in which case I'd urge you to consider
changing it for future asm patches. You seem to left-align all operands
after the instruction, like this:

mova [address], m0
mova m10,       [address]

My understanding is that we typically right-align the first, and left-align
the rest, to decrease the amount of spacings between operands. This is more
complex with three-operand instructions, so some people have some alignment
for the middle operand (causing there to be spacing anyway, bleh), or just
consider everything after the first as one (but then the third might not be
aligned), but anyway, the more consistent way (compared to other yasm code)
for the above code block would be:

mova [address], m0
mova       m10, [address]

There's various opinions on how mxx vs. mx should be handled, and I don't
really have much of an opinion. That is, both of these are correct:

mova        m1, m0
mova       m10, m11

vs.

mova       m1,  m0
mova       m10, m11

Again, if it was intentional, don't worry about it, it's fine, keep doing
it - but if it wasn't, consider changing the style so that at least the
left-most operand for each assembly instruction is right-aligned instead of
left-aligned. Left-aligning mxx vs mx is fine, but I'm more worried about
the long [address] things (like [blockq+123]), causing mx to be very left.
I'd prefer this:

[..]
   movq            m0, [pixelsq]
   add        pixelsq, linesizeq
   movq            m1, [pixelsq]
[..]
   punpcklbw       m2, m4
   punpcklbw       m3, m4
   mova  [blockq    ], m0
   mova  [blockq+16 ], m1
[..]

And again, patch OK, this isn't really important, just a random comment you
can disregard if you prefer your way.

Ronald