Mac Assembler

Mac Assembler and Linux cleanup
BUG=20749
TEST=play mac video... it should be time times faster rendering and visibly smoother


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26070

[scherkus (not reviewing), 2009-09-02 23:23:34]

I'm really sad at the amount of duplication we have going on here... we have
three totally separate copies of nearly the exact same assembly code but no easy
way to fix it short of using yasm/nasm.

we could have ~2 copies by using some macro magic to get around the mac _ prefix
issue:

#define STRINGIFY(x) #x
#if defined(OS_MAC)
#define GLOBAL(x) STRINGIFY(_##x)
#else
#define GLOBAL(x) STRINGIFY(x)
#endif

Then have something like:
"movq " GLOBAL(kCoefficientsRgbY) "(,%eax,8),%mm1\n"

it'll evaluate to:
"movq " "_kCoefficientsRgbY" "(,%eax,8),%mm1\n"

which is the same as:
"movq _kCoefficientsRgbY(,%eax,8),%mm1\n"

it's not pretty but it would work heh

http://codereview.chromium.org/180066/diff/5004/4004
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/180066/diff/5004/4004#newcode40
Line 40: #define MMX_ALIGNED(var) var __attribute__ ((section ("text")))
__attribute__ ((aligned(16)))
nit: > 80 chars

break onto next line.. you can probably align at the __attribute__

http://codereview.chromium.org/180066/diff/5004/4004#newcode306
Line 306: "convertloop :\n"
nit: based off your function labels, perhaps this should be unindented and with
no space inbetween the label and :

http://codereview.chromium.org/180066/diff/5004/4004#newcode325
Line 325: "convertend :\n"
ditto

http://codereview.chromium.org/180066/diff/5004/4004#newcode329
Line 329: "and       ecx, 1\n"  // odd number of pixels?
is this a TODO statement or is this what this line is testing for?

http://codereview.chromium.org/180066/diff/5004/4004#newcode342
Line 342: "convertdone :\n"
ditto

http://codereview.chromium.org/180066/diff/5004/4004#newcode343
Line 343: 
nit: blank line (just going off what the rest of your code looks like)

http://codereview.chromium.org/180066/diff/5004/4004#newcode370
Line 370: "scaleloop :\n"
ditto

http://codereview.chromium.org/180066/diff/5004/4004#newcode396
Line 396: "scaleend :\n"
ditto

http://codereview.chromium.org/180066/diff/5004/4004#newcode400
Line 400: "and       ecx, 1\n"  // odd number of pixels?
same here.. is this a TODO or actually a check

http://codereview.chromium.org/180066/diff/5004/4004#newcode426
Line 426: "scaledone :\n"
ditto

http://codereview.chromium.org/180066/diff/5004/4002
File media/base/yuv_row_mac.cc (right):

http://codereview.chromium.org/180066/diff/5004/4002#newcode40
Line 40: #define MMX_ALIGNED(var) var __attribute__ ((section
("__TEXT,__text"))) __attribute__ ((aligned(16)))
nit: >80 chars

[fbarchard, 2009-09-03 00:16:43]

On 2009/09/02 23:23:34, scherkus wrote:
> I'm really sad at the amount of duplication we have going on here... we have
> three totally separate copies of nearly the exact same assembly code but no
easy
> way to fix it short of using yasm/nasm.

I'm not too worried about that... the main issue is trying to make it actually
work, which it doesnt, due to the linker not allowing table lookups.
The simple fix is change the link options.
But barring that, I might have to do drastically different code for mac. 
Currently, its just C, which is certainly different than MMX.

> 
> we could have ~2 copies by using some macro magic to get around the mac _
prefix
> issue:
> 
> #define STRINGIFY(x) #x
> #if defined(OS_MAC)
> #define GLOBAL(x) STRINGIFY(_##x)
> #else
> #define GLOBAL(x) STRINGIFY(x)
> #endif

premature to fix this.  If I go to C++ the labels will be completely different
or irrelevant.
Or I might do the table as .dw statements in assembly and the label would be the
same.
> 
> Then have something like:
> "movq " GLOBAL(kCoefficientsRgbY) "(,%eax,8),%mm1\n"
> 
> it'll evaluate to:
> "movq " "_kCoefficientsRgbY" "(,%eax,8),%mm1\n"
> 
> which is the same as:
> "movq _kCoefficientsRgbY(,%eax,8),%mm1\n"
> 
> it's not pretty but it would work heh
> 
> http://codereview.chromium.org/180066/diff/5004/4004
> File media/base/yuv_row_linux.cc (right):
> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode40
> Line 40: #define MMX_ALIGNED(var) var __attribute__ ((section ("text")))
> __attribute__ ((aligned(16)))
> nit: > 80 chars
> 
> break onto next line.. you can probably align at the __attribute__
> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode306
> Line 306: "convertloop :\n"
> nit: based off your function labels, perhaps this should be unindented and
with
> no space inbetween the label and :
> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode325
> Line 325: "convertend :\n"
> ditto
> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode329
> Line 329: "and       ecx, 1\n"  // odd number of pixels?
> is this a TODO statement or is this what this line is testing for?

no, and is an if() statement.  I'm testing a condition.  The comment says what
I'm testing for.
If it were a TODO it would be on its own line and follow normal style
TODO(fbarchard): something to do.

> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode342
> Line 342: "convertdone :\n"
> ditto
> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode343
> Line 343: 
> nit: blank line (just going off what the rest of your code looks like)
> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode370
> Line 370: "scaleloop :\n"
> ditto
> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode396
> Line 396: "scaleend :\n"
> ditto
> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode400
> Line 400: "and       ecx, 1\n"  // odd number of pixels?
> same here.. is this a TODO or actually a check
> 
> http://codereview.chromium.org/180066/diff/5004/4004#newcode426
> Line 426: "scaledone :\n"
> ditto
> 
> http://codereview.chromium.org/180066/diff/5004/4002
> File media/base/yuv_row_mac.cc (right):
> 
> http://codereview.chromium.org/180066/diff/5004/4002#newcode40
> Line 40: #define MMX_ALIGNED(var) var __attribute__ ((section
> ("__TEXT,__text"))) __attribute__ ((aligned(16)))
> nit: >80 chars

Done.

[scherkus (not reviewing), 2009-09-03 00:54:39]

not seeing updated code yet, but just spotted a typo in your TEST= description:

"should be time times faster" :)

[Mark Mentovai, 2009-09-03 01:53:36]

OK, for the Mac stuff...

You could use a thunk to grab the instruction pointer and then use
offsets relative to that, like:

  call Lthunk
Lthunk:
  pop %ebx
  lea _kCoefficientsRgbY-Lthunk(%ebx), %eax

but you're just about out of general-purpose registers.

Just about, but not quite.  There's still %esp.  One creative idea
would be to stash %esp somewhere in your custom prologue and restore
it in your epilogue.  I notice that you've got a five whole unused MMX
registers at your disposal.  Maybe save %esp in one of them?

That'll work for FastConvertYUVToRGB32Row.  ScaleYUVToRGB32Row looks a
little harder, since it seems like you were so pressed for registers
that you're grabbing one of your arguments from the stack in the loop.
 Maybe this will give you enough headroom to work something out.
Maybe you can save that sixth argument in an MMX register also.  It's
worth at least investigating what's possible here.

I have an additional suggestion, which unfortunately isn't a solution
to your problem: name your local labels beginning with a capital L.
That'll keep you from generating local symbols for those labels.  You
don't need those local symbols, and they make gdb stop at
"convertloop" when you ask it to "disass FastConvertYUVToRGB32Row".

Mark

Frank Barchard wrote:
> Reviewers: scherkus, Mark Mentovai,
>
> Message:
> On 2009/09/02 23:23:34, scherkus wrote:
>>
>> I'm really sad at the amount of duplication we have going on here...
>
> we have
>>
>> three totally separate copies of nearly the exact same assembly code
>
> but no easy
>>
>> way to fix it short of using yasm/nasm.
>
> I'm not too worried about that... the main issue is trying to make it
> actually work, which it doesnt, due to the linker not allowing table
> lookups.
> The simple fix is change the link options.
> But barring that, I might have to do drastically different code for mac.
> =A0Currently, its just C, which is certainly different than MMX.
>
>
>> we could have ~2 copies by using some macro magic to get around the
>
> mac _ prefix
>>
>> issue:
>
>> #define STRINGIFY(x) #x
>> #if defined(OS_MAC)
>> #define GLOBAL(x) STRINGIFY(_##x)
>> #else
>> #define GLOBAL(x) STRINGIFY(x)
>> #endif
>
> premature to fix this. =A0If I go to C++ the labels will be completely
> different or irrelevant.
> Or I might do the table as .dw statements in assembly and the label
> would be the same.
>
>> Then have something like:
>> "movq " GLOBAL(kCoefficientsRgbY) "(,%eax,8),%mm1\n"
>
>> it'll evaluate to:
>> "movq " "_kCoefficientsRgbY" "(,%eax,8),%mm1\n"
>
>> which is the same as:
>> "movq _kCoefficientsRgbY(,%eax,8),%mm1\n"
>
>> it's not pretty but it would work heh
>
>> http://codereview.chromium.org/180066/diff/5004/4004
>> File media/base/yuv_row_linux.cc (right):
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode40
>> Line 40: #define MMX_ALIGNED(var) var __attribute__ ((section
>
> ("text")))
>>
>> __attribute__ ((aligned(16)))
>> nit: > 80 chars
>
>> break onto next line.. you can probably align at the __attribute__
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode306
>> Line 306: "convertloop :\n"
>> nit: based off your function labels, perhaps this should be unindented
>
> and with
>>
>> no space inbetween the label and :
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode325
>> Line 325: "convertend :\n"
>> ditto
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode329
>> Line 329: "and =A0 =A0 =A0 ecx, 1\n" =A0// odd number of pixels?
>> is this a TODO statement or is this what this line is testing for?
>
> no, and is an if() statement. =A0I'm testing a condition. =A0The comment
> says what I'm testing for.
> If it were a TODO it would be on its own line and follow normal style
> TODO(fbarchard): something to do.
>
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode342
>> Line 342: "convertdone :\n"
>> ditto
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode343
>> Line 343:
>> nit: blank line (just going off what the rest of your code looks like)
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode370
>> Line 370: "scaleloop :\n"
>> ditto
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode396
>> Line 396: "scaleend :\n"
>> ditto
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode400
>> Line 400: "and =A0 =A0 =A0 ecx, 1\n" =A0// odd number of pixels?
>> same here.. is this a TODO or actually a check
>
>> http://codereview.chromium.org/180066/diff/5004/4004#newcode426
>> Line 426: "scaledone :\n"
>> ditto
>
>> http://codereview.chromium.org/180066/diff/5004/4002
>> File media/base/yuv_row_mac.cc (right):
>
>> http://codereview.chromium.org/180066/diff/5004/4002#newcode40
>> Line 40: #define MMX_ALIGNED(var) var __attribute__ ((section
>> ("__TEXT,__text"))) __attribute__ ((aligned(16)))
>> nit: >80 chars
>
> Done.
>
>
> Description:
> Mac Assembler and Linux cleanup
> BUG=3D20749
> TEST=3Dplay mac video... it should be time times faster rendering and
> visibly smoother
>
>
> Please review this at http://codereview.chromium.org/180066
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 media/base/yuv_row.h
> =A0M =A0 =A0 media/base/yuv_row_linux.cc
> =A0M =A0 =A0 media/base/yuv_row_mac.cc
>
>
>

[Amanda Walker, 2009-09-03 06:12:37]

On 2009/09/02 23:23:34, scherkus wrote:
> I'm really sad at the amount of duplication we have going on here... we have
> three totally separate copies of nearly the exact same assembly code but no
easy
> way to fix it short of using yasm/nasm.

what's in the way of using nasm? It's included with Xcode on the Mac, and is
easy enough to install on windows and linux.  If we solve the PC-relative
addressing problem for the table lookups, wouldn't that work on all 3 platforms?

[fbarchard1, 2009-09-03 07:52:59]

The main obstacles right now is that linux 64 bit and mac are C code.If we
cant figure out how to do the assembly, my fallback plan is to use
intrinsics.

On Wed, Sep 2, 2009 at 11:12 PM, <amanda@chromium.org> wrote:

> On 2009/09/02 23:23:34, scherkus wrote:
>
>> I'm really sad at the amount of duplication we have going on here...
>>
> we have
>
>> three totally separate copies of nearly the exact same assembly code
>>
> but no easy
>
>> way to fix it short of using yasm/nasm.
>>
>
> what's in the way of using nasm? It's included with Xcode on the Mac,
> and is easy enough to install on windows and linux.  If we solve the
> PC-relative addressing problem for the table lookups, wouldn't that work
> on all 3 platforms?
>
>
>
> http://codereview.chromium.org/180066
>

[fbarchard, 2009-09-04 20:53:08]

in this attempt, I did mmx with thunk for unscaled and C for scaled, just to see
how it looks

[sra, 2009-09-05 07:38:44]

http://codereview.chromium.org/180066/diff/11004/11005
File media/base/yuv_row_mac.cc (right):

http://codereview.chromium.org/180066/diff/11004/11005#newcode303
Line 303: "lea    _kCoefficientsRgbY-convertgeteip(%eax),%eax\n"
It is probably best to not pop the return address because that will confuse the
return address branch prediction. 


   call eip_to_eax
after_call:
    lea _kCoefficientsRgbY-after_call(%eax),%eax

eip_to_eax:
  mov (%esp),%eax
  ret

[fbarchard1, 2009-09-05 07:44:25]

On Sat, Sep 5, 2009 at 12:38 AM, <sra@chromium.org> wrote:

>
> http://codereview.chromium.org/180066/diff/11004/11005
> File media/base/yuv_row_mac.cc (right):
>
> http://codereview.chromium.org/180066/diff/11004/11005#newcode303
> Line 303: "lea    _kCoefficientsRgbY-convertgeteip(%eax),%eax\n"
> It is probably best to not pop the return address because that will
> confuse the return address branch prediction.
>
>
>   call eip_to_eax
> after_call:
>    lea _kCoefficientsRgbY-after_call(%eax),%eax
>
> eip_to_eax:
>  mov (%esp),%eax
>  ret


nice.

but I hope the stuff outside the loop doesnt matter too much.
do you spot the reason my table lookups wont build?

[Mark Mentovai, 2009-09-05 18:59:27]

http://codereview.chromium.org/180066/diff/11004/11005
File media/base/yuv_row_mac.cc (right):

http://codereview.chromium.org/180066/diff/11004/11005#newcode320
Line 320: "movq   $2048(%ecx,%eax,8),%mm0\n"
No $.  I think that's the problem in this version.  You wouldn't write
$0x24($esp) on line 305, and this is no different.  You don't want an immediate
value, you want base-relative.

We'll have to hope that the compiler emits the tables in Y-U-V order as written.
 It's a reasonable assumption, but it would be even more reasonable to write all
three tables as a single array of [768][4].

[fbarchard, 2009-09-08 21:43:53]

fixed table offsets and pop %eax

[fbarchard, 2009-09-08 23:26:52]

in this version, the convert is building and running the unittest again.  I put
a place holder C version for the scaler.  But ajwong is concerned that using esp
is dangerous for signals - that they will use esp as a stack.  Is this true?

[awong, 2009-09-08 23:42:33]

To clarify, IIRC, when the kernel invokes a signal handler, by default, it
pushes onto the current stack unless you specifically specify SA_ONSTACK
during sigaction() and call sigaltstack().  If a signal is caught while
executing this loop, things would go bad.

On Tue, Sep 8, 2009 at 4:26 PM, <fbarchard@chromium.org> wrote:

> in this version, the convert is building and running the unittest again.
>  I put a place holder C version for the scaler.  But ajwong is concerned
> that using esp is dangerous for signals - that they will use esp as a
> stack.  Is this true?
>
>
> http://codereview.chromium.org/180066
>

[Amanda Walker, 2009-09-09 01:31:25]

Yes.  This was the essence of my "interrupts disabled" comment
earlier.  Physical interrupts aren't the problem in a modern OS, but
signals (and potentially other events that can preempt the current
thread) are free to use the stack.

--Amanda

On Tue, Sep 8, 2009 at 7:36 PM, Albert J. Wong (=E7=8E=8B=E9=87=8D=E5=82=91=
)<ajwong@chromium.org> wrote:
> To clarify, IIRC, when the kernel invokes a signal handler, by default, i=
t
> pushes onto the current stack unless you specifically specify SA_ONSTACK
> during sigaction() and call sigaltstack(). =C2=A0If a signal is caught wh=
ile
> executing this loop, things would go bad.
>
> On Tue, Sep 8, 2009 at 4:26 PM, <fbarchard@chromium.org> wrote:
>>
>> in this version, the convert is building and running the unittest again.
>> =C2=A0I put a place holder C version for the scaler. =C2=A0But ajwong is=
 concerned
>> that using esp is dangerous for signals - that they will use esp as a
>> stack. =C2=A0Is this true?
>>
>> http://codereview.chromium.org/180066
>
>



--=20
"Portability is generally the result of advance planning rather than trench
warfare involving #ifdef" -- Henry Spencer (1992)

[fbarchard, 2009-09-09 02:13:40]

So we're back to the drawing board.
Can't use eip on 32 bit asm.
Can't use esp for general purpose register.
My options get worse from here:
disable interupts
memcpy tables to the stack.
self modifying code
make code use one less register / one more stack variable.

It remains unclear to me why this is a relocation at all, since it should be
resolved at link time.

[sra1, 2009-09-09 06:01:14]

Is this code linked into the main executable or a shared library? I don't
know if this is why you have a reloc problem.  Many versions of unix use
position independent code so a .so file can be loaded at any address.  If
you have pic code then you can't have any absolute relocs.
Write the function in C++ using intrinsics and time that version.  Look at
the generated code to see how it handles the tables.
Does compiling it with -O3 or other flags make it fast enough?

Something has been puzzling me.  The code we were discussing on another
thread had a variable stride that took a register.  This code has unit
stride.  Why does this code still have a no spare register?  (Or are they
different functions)

What is the critical resource in this loop?

On Tue, Sep 8, 2009 at 7:13 PM, <fbarchard@chromium.org> wrote:

> So we're back to the drawing board.
> Can't use eip on 32 bit asm.
> Can't use esp for general purpose register.
> My options get worse from here:
> disable interupts
> memcpy tables to the stack.
> self modifying code
> make code use one less register / one more stack variable.
>
> It remains unclear to me why this is a relocation at all, since it
> should be resolved at link time.
>
>
>
> http://codereview.chromium.org/180066
>

[fbarchard, 2009-09-11 18:19:53]

On 2009/09/09 06:01:14, sra_google.com wrote:
> Is this code linked into the main executable or a shared library?
Its in the media pipeline renderer, which I believe is a shared library.

> I don't
> know if this is why you have a reloc problem.  Many versions of unix use
> position independent code so a .so file can be loaded at any address.  If
> you have pic code then you can't have any absolute relocs.

shouldnt it be resolved to an absolute address at link time?

> Write the function in C++ using intrinsics and time that version.  Look at
> the generated code to see how it handles the tables.

I've tried that on windows and the performance was substantially slower, due to
register spill.  I can't do substantial work on Mac until I get my own... I'm
just porting and using try server.

> Does compiling it with -O3 or other flags make it fast enough?

no

> 
> Something has been puzzling me.  The code we were discussing on another
> thread had a variable stride that took a register.  This code has unit
> stride.  Why does this code still have a no spare register?  (Or are they
> different functions)

there are 5 variations of the function, depending on rotation and scaling flags.
 the fastest version has 1 'spare' register, which I used to improve
performance, by fetching 2 pixels at a time.

> 
> What is the critical resource in this loop?

memory fetches.  If the video is high def, the reads are the bottleneck.  which
is why its important to keep the rest of it using registers.

I now have code that uses a stack variable for the loop counter.
try server shows the old code that uses esp (unsafe) was 54 ms vs the new code
is 74 ms.  Its not a very accurate benchmark, but it shows a slowdown.  The C
code is about 200 ms.
I'll probably go with this for now.  It has 2 drawbacks:
1. Mac is different than Windows or Linux, increasing support cost for any code
changes.
2. Mac has a performance disadvantage compared to Linux/Windows and will require
a higher end CPU.  Fortunately, all Macs have at least a 'Core' CPU.

[awong, 2009-09-12 00:55:28]

Couple of small style nits. Fix those and this latest patch looks sane enough
for a commit.  The linux changes look like just a pure syntax conversion.  The
mac changes read sensibly enough to me.  Even though it's a bit less efficient,
it's still a significant win over the status quo.

http://codereview.chromium.org/180066/diff/21013/13015
File media/base/yuv_row.h (right):

http://codereview.chromium.org/180066/diff/21013/13015#newcode66
Line 66: #if defined(_MSC_VER) || defined(ARCH_CPU_X86)
Use OS_WIN instead of _MSC_VER.  Same later.

http://codereview.chromium.org/180066/diff/21013/13014
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/180066/diff/21013/13014#newcode40
Line 40: #define MMX_ALIGNED(var) var __attribute__ ((section ("text")))
__attribute__ ((aligned(16)))
If we don't need to explicitly put this in the text segment, I would, just
remove that attribute.

[awong, 2009-09-12 00:56:15]

In otherwords, LGTM.

On 2009/09/12 00:55:28, awong wrote:
> Couple of small style nits. Fix those and this latest patch looks sane enough
> for a commit.  The linux changes look like just a pure syntax conversion.  The
> mac changes read sensibly enough to me.  Even though it's a bit less
efficient,
> it's still a significant win over the status quo.
> 
> http://codereview.chromium.org/180066/diff/21013/13015
> File media/base/yuv_row.h (right):
> 
> http://codereview.chromium.org/180066/diff/21013/13015#newcode66
> Line 66: #if defined(_MSC_VER) || defined(ARCH_CPU_X86)
> Use OS_WIN instead of _MSC_VER.  Same later.
> 
> http://codereview.chromium.org/180066/diff/21013/13014
> File media/base/yuv_row_linux.cc (right):
> 
> http://codereview.chromium.org/180066/diff/21013/13014#newcode40
> Line 40: #define MMX_ALIGNED(var) var __attribute__ ((section ("text")))
> __attribute__ ((aligned(16)))
> If we don't need to explicitly put this in the text segment, I would, just
> remove that attribute.

[fbarchard, 2009-09-12 01:03:58]

I'd like to keep text for one release.  It has no drawback, allows me to move
forward with thunking, and documents how to do a none obvious section for mac.
I'll be following up this release with more improvements soon enough... removing
the extra pixel, 64 bit, more versions for rotation, improvements in performance
if possible...

[awong, 2009-09-12 02:40:58]

Okay, if you're going to keep experimenting, go ahead with the checkin.

On Fri, Sep 11, 2009 at 6:03 PM, <fbarchard@chromium.org> wrote:

> I'd like to keep text for one release.  It has no drawback, allows me to
> move
> forward with thunking, and documents how to do a none obvious section for
> mac.
> I'll be following up this release with more improvements soon enough...
> removing
> the extra pixel, 64 bit, more versions for rotation, improvements in
> performance
> if possible...
>
>
>
> http://codereview.chromium.org/180066
>