Implement a NEON version of the RGBA gaussian blur. This shows a 9-15% speedup on Nexus-10.

Implement a NEON version of the RGBA gaussian blur. This shows a 9-15% speedup on Nexus-10.

R=mtklein@google.com, mtklein

before:
running bench [640 480]             blur_image_filter_large_10.00_10.00    8888: cmsecs =  33063.23
running bench [640 480]             blur_image_filter_small_10.00_10.00    8888: cmsecs =  32800.25
running bench [640 480]               blur_image_filter_large_1.00_1.00    8888: cmsecs =  33017.88
running bench [640 480]               blur_image_filter_small_1.00_1.00    8888: cmsecs =  32743.35
running bench [640 480]               blur_image_filter_large_0.00_1.00    8888: cmsecs =  21024.04
running bench [640 480]              blur_image_filter_large_0.00_10.00    8888: cmsecs =  22904.15
running bench [640 480]               blur_image_filter_large_1.00_0.00    8888: cmsecs =  18738.08
running bench [640 480]              blur_image_filter_large_10.00_0.00    8888: cmsecs =  18798.98

after:
running bench [640 480]             blur_image_filter_large_10.00_10.00    8888: cmsecs =  30180.96
running bench [640 480]             blur_image_filter_small_10.00_10.00    8888: cmsecs =  29861.90
running bench [640 480]               blur_image_filter_large_1.00_1.00    8888: cmsecs =  30178.98
running bench [640 480]               blur_image_filter_small_1.00_1.00    8888: cmsecs =  29911.25
running bench [640 480]               blur_image_filter_large_0.00_1.00    8888: cmsecs =  19344.35
running bench [640 480]              blur_image_filter_large_0.00_10.00    8888: cmsecs =  19957.07
running bench [640 480]               blur_image_filter_large_1.00_0.00    8888: cmsecs =  17158.84
running bench [640 480]              blur_image_filter_large_10.00_0.00    8888: cmsecs =  17330.73

Committed: https://code.google.com/p/skia/source/detail?r=12486

[Stephen White, 2013-12-02 21:36:59]

mtklein@: PTAL. Thanks!

[mtklein, 2013-12-03 20:22:13]

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
File src/opts/SkBlurImage_opts_neon.cpp (right):

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:28: uint8x8_t v8 =
vreinterpret_u8_u32(vdup_n_u32(a));
Do you mind adding in comment crutches for me again?

I think this is:

// ARGB -> A R G B A R G B
uint8x8_t v8 = vreinterpret_u8_u32(vdup_n_u32(a));

// A R G B A R G B -> 0A 0R 0G 0B 0A 0R 0G 0B -> 0A 0R 0G 0B
const uint16x4_t v16 = vget_low_u16(vmovl_u8(v8));

// 0A 0R 0G 0B -> 000A 000R 000G 000B
return vmovl_u16(v16);

Can we just use v16 directly later instead of promoting it up to 128 bit?

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:34: void SkBoxBlur_SSE2(const SkPMColor* src,
int srcStride, SkPMColor* dst, int kernelSize,
Given that we're clearly copying and pasting here (_SSE2, here and below), want
to try something crazy?  You feeling ambitious?  Want to try to unify the
codebase for the SSE and NEON versions?  The structural code is pretty much
verbatim, and there are some very strong correspondences between the SSE and
NEON we're using (though I admit, if we use a 64-bit NEON register as I hoped
above, that may disappear).

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:69: const SkPMColor* l = sptr - leftOffset *
srcStrideX;
I've only just noticed, we used SkColor here in the SSE code, and we mix
SkPMColor and SkColor in ways that don't quite make sense to me.  Given that the
interface always speaks SkPMColor, seems simpler just to make everything work
with SkPMColor and make sure we're not interacting between components in a way
that the packing order matters?

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:93: *boxBlurX = SkBoxBlur_SSE2<kX, kX>;
_SSE2 here too

[Stephen White, 2013-12-04 15:18:41]

Thanks for your review.

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
File src/opts/SkBlurImage_opts_neon.cpp (right):

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:28: uint8x8_t v8 =
vreinterpret_u8_u32(vdup_n_u32(a));
On 2013/12/03 20:22:13, mtklein wrote:
> Do you mind adding in comment crutches for me again?

Done. I put in some brackets to indicate the length of the register, and used
spaces instead of 0s for clarity. Let me know if you think it's more readable
this way.

> // A R G B A R G B -> 0A 0R 0G 0B 0A 0R 0G 0B -> 0A 0R 0G 0B
> const uint16x4_t v16 = vget_low_u16(vmovl_u8(v8));
> 
> // 0A 0R 0G 0B -> 000A 000R 000G 000B
> return vmovl_u16(v16);
> 
> Can we just use v16 directly later instead of promoting it up to 128 bit?

The 4 sums that this is used in all have to be 32bit anyway, so we need to
promote it regardless.

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:34: void SkBoxBlur_SSE2(const SkPMColor* src,
int srcStride, SkPMColor* dst, int kernelSize,
On 2013/12/03 20:22:13, mtklein wrote:
> Given that we're clearly copying and pasting here (_SSE2, here and below)

Whoops, busted. :) Fixed.

>, want
> to try something crazy?  You feeling ambitious?  Want to try to unify the
> codebase for the SSE and NEON versions?  The structural code is pretty much
> verbatim, and there are some very strong correspondences between the SSE and
> NEON we're using (though I admit, if we use a 64-bit NEON register as I hoped
> above, that may disappear).

I thought about it, but came to the conclusion that the scaffolding might well
result in more lines of code than it would save.

One possible refactoring that might work would be to move the outer loop and
stride calculations out of the implementations, and just make the functions
operate on a single scan line. This is how most of the other opts are written.
I'd prefer to do that as a followup, though.

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:69: const SkPMColor* l = sptr - leftOffset *
srcStrideX;
On 2013/12/03 20:22:13, mtklein wrote:
> I've only just noticed, we used SkColor here in the SSE code, and we mix
> SkPMColor and SkColor in ways that don't quite make sense to me.  Given that
the
> interface always speaks SkPMColor, seems simpler just to make everything work
> with SkPMColor and make sure we're not interacting between components in a way
> that the packing order matters?

Thanks, done.

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:93: *boxBlurX = SkBoxBlur_SSE2<kX, kX>;
On 2013/12/03 20:22:13, mtklein wrote:
> _SSE2 here too

Fixed.

[mtklein, 2013-12-04 16:52:23]

LGTM with one readability suggestion.  See what you think before submitting?

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
File src/opts/SkBlurImage_opts_neon.cpp (right):

https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:34: void SkBoxBlur_SSE2(const SkPMColor* src,
int srcStride, SkPMColor* dst, int kernelSize,
On 2013/12/04 15:18:41, Stephen White wrote:
> On 2013/12/03 20:22:13, mtklein wrote:
> > Given that we're clearly copying and pasting here (_SSE2, here and below)
> 
> Whoops, busted. :) Fixed.
> 
> >, want
> > to try something crazy?  You feeling ambitious?  Want to try to unify the
> > codebase for the SSE and NEON versions?  The structural code is pretty much
> > verbatim, and there are some very strong correspondences between the SSE and
> > NEON we're using (though I admit, if we use a 64-bit NEON register as I
hoped
> > above, that may disappear).
> 
> I thought about it, but came to the conclusion that the scaffolding might well
> result in more lines of code than it would save.
> 
> One possible refactoring that might work would be to move the outer loop and
> stride calculations out of the implementations, and just make the functions
> operate on a single scan line. This is how most of the other opts are written.
> I'd prefer to do that as a followup, though.

SGTM

https://codereview.chromium.org/99933004/diff/40001/src/opts/SkBlurImage_opts...
File src/opts/SkBlurImage_opts_neon.cpp (right):

https://codereview.chromium.org/99933004/diff/40001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_neon.cpp:30: // ( A R G B A R G B ) -> (   A   R   G  
B   A   R   G   B ) -> (   A   R   G   B )
Maybe use _ for 0's leaving spaces for spacing?

[Stephen White, 2013-12-04 18:17:56]

On 2013/12/04 16:52:23, mtklein wrote:
> LGTM with one readability suggestion.  See what you think before submitting?
> 
>
https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
> File src/opts/SkBlurImage_opts_neon.cpp (right):
> 
>
https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
> src/opts/SkBlurImage_opts_neon.cpp:34: void SkBoxBlur_SSE2(const SkPMColor*
src,
> int srcStride, SkPMColor* dst, int kernelSize,
> On 2013/12/04 15:18:41, Stephen White wrote:
> > On 2013/12/03 20:22:13, mtklein wrote:
> > > Given that we're clearly copying and pasting here (_SSE2, here and below)
> > 
> > Whoops, busted. :) Fixed.
> > 
> > >, want
> > > to try something crazy?  You feeling ambitious?  Want to try to unify the
> > > codebase for the SSE and NEON versions?  The structural code is pretty
much
> > > verbatim, and there are some very strong correspondences between the SSE
and
> > > NEON we're using (though I admit, if we use a 64-bit NEON register as I
> hoped
> > > above, that may disappear).
> > 
> > I thought about it, but came to the conclusion that the scaffolding might
well
> > result in more lines of code than it would save.
> > 
> > One possible refactoring that might work would be to move the outer loop and
> > stride calculations out of the implementations, and just make the functions
> > operate on a single scan line. This is how most of the other opts are
written.
> > I'd prefer to do that as a followup, though.
> 
> SGTM
> 
>
https://codereview.chromium.org/99933004/diff/40001/src/opts/SkBlurImage_opts...
> File src/opts/SkBlurImage_opts_neon.cpp (right):
> 
>
https://codereview.chromium.org/99933004/diff/40001/src/opts/SkBlurImage_opts...
> src/opts/SkBlurImage_opts_neon.cpp:30: // ( A R G B A R G B ) -> (   A   R   G
 
> B   A   R   G   B ) -> (   A   R   G   B )
> Maybe use _ for 0's leaving spaces for spacing?

Actually this made me like 0's better. Fixed. :)

[Stephen White, 2013-12-04 18:19:50]

Committed patchset #4 manually as r12486 (presubmit successful).

[mtklein, 2013-12-04 18:20:13]

On 2013/12/04 18:17:56, Stephen White wrote:
> On 2013/12/04 16:52:23, mtklein wrote:
> > LGTM with one readability suggestion.  See what you think before submitting?
> > 
> >
>
https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
> > File src/opts/SkBlurImage_opts_neon.cpp (right):
> > 
> >
>
https://codereview.chromium.org/99933004/diff/20001/src/opts/SkBlurImage_opts...
> > src/opts/SkBlurImage_opts_neon.cpp:34: void SkBoxBlur_SSE2(const SkPMColor*
> src,
> > int srcStride, SkPMColor* dst, int kernelSize,
> > On 2013/12/04 15:18:41, Stephen White wrote:
> > > On 2013/12/03 20:22:13, mtklein wrote:
> > > > Given that we're clearly copying and pasting here (_SSE2, here and
below)
> > > 
> > > Whoops, busted. :) Fixed.
> > > 
> > > >, want
> > > > to try something crazy?  You feeling ambitious?  Want to try to unify
the
> > > > codebase for the SSE and NEON versions?  The structural code is pretty
> much
> > > > verbatim, and there are some very strong correspondences between the SSE
> and
> > > > NEON we're using (though I admit, if we use a 64-bit NEON register as I
> > hoped
> > > > above, that may disappear).
> > > 
> > > I thought about it, but came to the conclusion that the scaffolding might
> well
> > > result in more lines of code than it would save.
> > > 
> > > One possible refactoring that might work would be to move the outer loop
and
> > > stride calculations out of the implementations, and just make the
functions
> > > operate on a single scan line. This is how most of the other opts are
> written.
> > > I'd prefer to do that as a followup, though.
> > 
> > SGTM
> > 
> >
>
https://codereview.chromium.org/99933004/diff/40001/src/opts/SkBlurImage_opts...
> > File src/opts/SkBlurImage_opts_neon.cpp (right):
> > 
> >
>
https://codereview.chromium.org/99933004/diff/40001/src/opts/SkBlurImage_opts...
> > src/opts/SkBlurImage_opts_neon.cpp:30: // ( A R G B A R G B ) -> (   A   R  
G
>  
> > B   A   R   G   B ) -> (   A   R   G   B )
> > Maybe use _ for 0's leaving spaces for spacing?
> 
> Actually this made me like 0's better. Fixed. :)

:)  LGTM