ARM Skia NEON patches - 16/17 - Blitmask

ARM Skia NEON patches - 16/17 - Blitmask

Blitmask: NEON optimised version of the D32_A8 functions
    
    Here are the microbenchmark results I got for the D32_A8
    functions:
    
    Cortex-A9:
    ==========
    +-------+--------+--------+--------+
    | count | Black  | Opaque | Color  |
    +-------+--------+--------+--------+
    | 1     | -14%   | -39,5% | -37,5% |
    +-------+--------+--------+--------+
    | 2     | -3%    | -29,9% | -25%   |
    +-------+--------+--------+--------+
    | 4     | -11,3% | -22%   | -14,5% |
    +-------+--------+--------+--------+
    | 8     | +128%  | +66,6% | +105%  |
    +-------+--------+--------+--------+
    | 16    | +159%  | +102%  | +149%  |
    +-------+--------+--------+--------+
    | 64    | +189%  | +136%  | +189%  |
    +-------+--------+--------+--------+
    | 256   | +126%  | +102%  | +149%  |
    +-------+--------+--------+--------+
    | 1024  | +67,5% | +81,4% | +123%  |
    +-------+--------+--------+--------+
    
    Cortex-A15:
    ===========
    +-------+--------+--------+--------+
    | count | Black  | Opaque | Color  |
    +-------+--------+--------+--------+
    | 1     | -24%   | -46,5% | -37,5% |
    +-------+--------+--------+--------+
    | 2     | -18,5% | -35,5% | -28%   |
    +-------+--------+--------+--------+
    | 4     | -5,2%  | -17,5% | -15,5% |
    +-------+--------+--------+--------+
    | 8     | +72%   | +65,8% | +84,7% |
    +-------+--------+--------+--------+
    | 16    | +168%  | +117%  | +149%  |
    +-------+--------+--------+--------+
    | 64    | +165%  | +110%  | +145%  |
    +-------+--------+--------+--------+
    | 256   | +106%  | +99,6% | +141%  |
    +-------+--------+--------+--------+
    | 1024  | +93,7% | +94,7% | +130%  |
    +-------+--------+--------+--------+
    
    Blitmask: add NEON optimised PlatformBlitRowProcs16
    
    Here are the microbenchmark results (speedup vs. C code):
    
    +-------+-----------------+-----------------+
    |       |    Cortex-A9    |   Cortex-A15    |
    | count +--------+--------+--------+--------+
    |       | Blend  | Opaque | Blend  | Opaque |
    +-------+--------+--------+--------+--------+
    | 1     | -19,2% | -36,7% | -33,6% | -44,7% |
    +-------+--------+--------+--------+--------+
    | 2     | -12,6% | -27,8% | -39%   | -48%   |
    +-------+--------+--------+--------+--------+
    | 4     | -11,5% | -21,6% | -37,7% | -44,3% |
    +-------+--------+--------+--------+--------+
    | 8     | +141%  | +59,7% | +123%  | +48,7% |
    +-------+--------+--------+--------+--------+
    | 16    | +213%  | +119%  | +214%  | +121%  |
    +-------+--------+--------+--------+--------+
    | 64    | +212%  | +105%  | +242%  | +167%  |
    +-------+--------+--------+--------+--------+
    | 256   | +289%  | +167%  | +249%  | +207%  |
    +-------+--------+--------+--------+--------+
    | 1024  | +273%  | +169%  | +146%  | +220%  |
    +-------+--------+--------+--------+--------+
    
    Signed-off-by: Kévin PETIT <kevin.petit@arm.com>;

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=12420

[kevin.petit.not.used.account, 2013-11-11 15:13:01]

I've rebased, cleaned a little and grouped with similar code.

This code has no mismatches and brings up to 30% speed improvement on text
rendering.

[mtklein, 2013-11-11 17:17:03]

https://codereview.chromium.org/23719002/diff/7001/src/core/SkBlitMask_D32.cpp
File src/core/SkBlitMask_D32.cpp (right):

https://codereview.chromium.org/23719002/diff/7001/src/core/SkBlitMask_D32.cp...
src/core/SkBlitMask_D32.cpp:219: SkBlitMask::ColorProc D32_A8_Factory(SkColor
color) {
Having to make this change makes me concerned that we don't have the right API.

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
File src/opts/SkBlitMask_opts_arm.cpp (right):

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
src/opts/SkBlitMask_opts_arm.cpp:8: extern SkBlitMask::ColorProc
D32_A8_Factory(SkColor color);
Can we do these using the already exported SkBlitMask::{ColorFactory,
PlatformColorProcs}?  Making this guy non-static seems like we're just slicing
right through any abstraction barrier we've tried to set up here.

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
src/opts/SkBlitMask_opts_arm.cpp:11: extern void
SkBlitLCD16OpaqueRow_neon(SkPMColor dst[], const uint16_t src[],
Add a header for these two?

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
src/opts/SkBlitMask_opts_arm.cpp:21: switch (dstConfig) {
Given the limited support we've got here, something like this seems
significantly easier to follow:

if (SkBitmap::kARGB_8888_Config == dst_config &&
    SkMask::kA8_Format == maskFormat) {
   return ...;
}
return NULL;

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkColor_opts_neon.h
File src/opts/SkColor_opts_neon.h (right):

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkColor_opts_neon...
src/opts/SkColor_opts_neon.h:69: static inline uint8x8_t
SkBlend32_neon8(uint8x8_t src, uint8x8_t dst, uint16x8_t scale) {
Add a brief comment like for 16->32 and 32->16 above?

[djsollen, 2013-11-11 17:21:45]

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkColor_opts_neon.h
File src/opts/SkColor_opts_neon.h (right):

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkColor_opts_neon...
src/opts/SkColor_opts_neon.h:5: #include "SkColorPriv.h"
why do you need this include?

[reed1, 2013-11-11 17:32:55]

What is 'count' in your perf graphs? mask-width? Perhaps we should consider ways
to be optimal for both sides of this 8 boundary... fast for small (C) and fast
for big (neon)...

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
File src/opts/SkBlitMask_opts_arm.cpp (right):

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
src/opts/SkBlitMask_opts_arm.cpp:20: SkColor color) {
How about we just add the following as the first check in this function:

if (!sk_cpu_arm_has_neon()) {
    return NULL;
}

Then the rest of this code can just be switch statements etc, assuming neon is
valid. This will free us in several ways:

1. no need to import other people's factories (e.g. D32_A8_Factor)
2. no need to follow any predetermined naming convention (e.g. _neon)

The design goal for runtime PlatformProcs is to give maximal freedom to
platform-specific optimizations, while giving maximal isolation between that
logic and the core.

[kevin.petit.not.used.account, 2013-11-11 18:10:34]

Patch Set 4 addresses review comments.

https://codereview.chromium.org/23719002/diff/7001/src/core/SkBlitMask_D32.cpp
File src/core/SkBlitMask_D32.cpp (right):

https://codereview.chromium.org/23719002/diff/7001/src/core/SkBlitMask_D32.cp...
src/core/SkBlitMask_D32.cpp:219: SkBlitMask::ColorProc D32_A8_Factory(SkColor
color) {
Fixed. This wasn't necessary.

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
File src/opts/SkBlitMask_opts_arm.cpp (right):

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
src/opts/SkBlitMask_opts_arm.cpp:8: extern SkBlitMask::ColorProc
D32_A8_Factory(SkColor color);
Yes, you're right. Done.

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
src/opts/SkBlitMask_opts_arm.cpp:11: extern void
SkBlitLCD16OpaqueRow_neon(SkPMColor dst[], const uint16_t src[],
On 2013/11/11 17:17:04, mtklein wrote:
> Add a header for these two?

Done.

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
src/opts/SkBlitMask_opts_arm.cpp:20: SkColor color) {
Thanks for the tip. Done.

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkBlitMask_opts_a...
src/opts/SkBlitMask_opts_arm.cpp:21: switch (dstConfig) {
On 2013/11/11 17:17:04, mtklein wrote:
> Given the limited support we've got here, something like this seems
> significantly easier to follow:
> 
> if (SkBitmap::kARGB_8888_Config == dst_config &&
>     SkMask::kA8_Format == maskFormat) {
>    return ...;
> }
> return NULL;

Done.

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkColor_opts_neon.h
File src/opts/SkColor_opts_neon.h (right):

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkColor_opts_neon...
src/opts/SkColor_opts_neon.h:5: #include "SkColorPriv.h"
This is here to fix a pre-existing mistake. There is code in this file using the
SK_?16_* defines. It didn't show up because all files including this one were
including SkColorPriv.h before.

https://codereview.chromium.org/23719002/diff/7001/src/opts/SkColor_opts_neon...
src/opts/SkColor_opts_neon.h:69: static inline uint8x8_t
SkBlend32_neon8(uint8x8_t src, uint8x8_t dst, uint16x8_t scale) {
On 2013/11/11 17:17:04, mtklein wrote:
> Add a brief comment like for 16->32 and 32->16 above?

Done.

[kevin.petit.not.used.account, 2013-11-11 18:20:25]

On 2013/11/11 17:32:55, reed1 wrote:
> What is 'count' in your perf graphs? mask-width?

Yes.

> Perhaps we should consider ways
> to be optimal for both sides of this 8 boundary... fast for small (C) and fast
> for big (neon)...

That's what the code in this review does. Under 8: C, over: neon. I remember
having struggled a lot to try and reconcile the two. I do have an alternate
patch that has almost no regression under 8 but also a much much smaller speedup
(to the point where having the code altogether isn't anymore that interesting).

Passing the width to the factory may allow to have the best of both sides but to
be honest I haven't tried that approach.

[kevin.petit.not.used.account, 2013-11-25 17:00:19]

ping.

Any more comments on this?

[mtklein, 2013-11-26 14:44:00]

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
File src/opts/SkBlitMask_opts_arm.cpp (right):

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
src/opts/SkBlitMask_opts_arm.cpp:6: #include "SkBlitMask_opts_arm_neon.h"
Looks like this file is missing?

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
src/opts/SkBlitMask_opts_arm.cpp:21: 
stray newline here

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
File src/opts/SkBlitMask_opts_arm_neon.cpp (right):

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
src/opts/SkBlitMask_opts_arm_neon.cpp:5: static void D32_A8_Black_neon(void*
SK_RESTRICT dst, size_t dstRB,
Typically we make static methods all_lowercase_like_this.  There's not a special
reason not too, right?

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
src/opts/SkBlitMask_opts_arm_neon.cpp:40: static void D32_A8_Opaque_neon(void*
SK_RESTRICT dst, size_t dstRB,
Can you use templates to share the bulk of the code between this and the Color
version below?  They're so similar it's even hard to see where they differ at a
glance.

[kevin.petit.not.used.account, 2013-11-26 16:23:32]

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
File src/opts/SkBlitMask_opts_arm.cpp (right):

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
src/opts/SkBlitMask_opts_arm.cpp:6: #include "SkBlitMask_opts_arm_neon.h"
On 2013/11/26 14:44:01, mtklein wrote:
> Looks like this file is missing?

Done.

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
src/opts/SkBlitMask_opts_arm.cpp:21: 
On 2013/11/26 14:44:01, mtklein wrote:
> stray newline here

Done.

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
File src/opts/SkBlitMask_opts_arm_neon.cpp (right):

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
src/opts/SkBlitMask_opts_arm_neon.cpp:5: static void D32_A8_Black_neon(void*
SK_RESTRICT dst, size_t dstRB,
On 2013/11/26 14:44:01, mtklein wrote:
> Typically we make static methods all_lowercase_like_this.  There's not a
special
> reason not too, right?

I did that for consistency with all the static functions in
src/core/SkBlitMask_D32.cpp which all use uppercase letters.

https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
src/opts/SkBlitMask_opts_arm_neon.cpp:40: static void D32_A8_Opaque_neon(void*
SK_RESTRICT dst, size_t dstRB,
On 2013/11/26 14:44:01, mtklein wrote:
> Can you use templates to share the bulk of the code between this and the Color
> version below?  They're so similar it's even hard to see where they differ at
a
> glance.

Done. You're right, that was a bit silly to not have a template.

[mtklein, 2013-11-26 20:37:38]

On 2013/11/26 16:23:32, kevin.petit.arm wrote:
>
https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
> File src/opts/SkBlitMask_opts_arm.cpp (right):
> 
>
https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
> src/opts/SkBlitMask_opts_arm.cpp:6: #include "SkBlitMask_opts_arm_neon.h"
> On 2013/11/26 14:44:01, mtklein wrote:
> > Looks like this file is missing?
> 
> Done.
> 
>
https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
> src/opts/SkBlitMask_opts_arm.cpp:21: 
> On 2013/11/26 14:44:01, mtklein wrote:
> > stray newline here
> 
> Done.
> 
>
https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
> File src/opts/SkBlitMask_opts_arm_neon.cpp (right):
> 
>
https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
> src/opts/SkBlitMask_opts_arm_neon.cpp:5: static void D32_A8_Black_neon(void*
> SK_RESTRICT dst, size_t dstRB,
> On 2013/11/26 14:44:01, mtklein wrote:
> > Typically we make static methods all_lowercase_like_this.  There's not a
> special
> > reason not too, right?
> 
> I did that for consistency with all the static functions in
> src/core/SkBlitMask_D32.cpp which all use uppercase letters.
> 
>
https://codereview.chromium.org/23719002/diff/27004/src/opts/SkBlitMask_opts_...
> src/opts/SkBlitMask_opts_arm_neon.cpp:40: static void D32_A8_Opaque_neon(void*
> SK_RESTRICT dst, size_t dstRB,
> On 2013/11/26 14:44:01, mtklein wrote:
> > Can you use templates to share the bulk of the code between this and the
Color
> > version below?  They're so similar it's even hard to see where they differ
at
> a
> > glance.
> 
> Done. You're right, that was a bit silly to not have a template.

lgtm

[commit-bot: I haz the power, 2013-11-27 17:00:31]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kevin.petit.arm@gmail.com/23719002/13...

[commit-bot: I haz the power, 2013-11-27 17:08:39]

Change committed as 12420