Issue 61643011: SSE2 implementation of RGBA box blurs. This yields ~2X perf improvement on

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+189 lines, -20 lines)			Patch
M	bench/BlurImageFilterBench.cpp	View	1 2 3 4 5	1 chunk	+4 lines, -0 lines	0 comments	Download
M	gyp/opts.gyp	View	1 2 3	3 chunks	+3 lines, -0 lines	0 comments	Download
M	src/effects/SkBlurImageFilter.cpp	View	1 2 3 4	3 chunks	+8 lines, -20 lines	0 comments	Download
A	src/opts/SkBlurImage_opts.h	View	1 2 3 4	1 chunk	+19 lines, -0 lines	0 comments	Download
A	src/opts/SkBlurImage_opts_SSE2.h	View	1 2 3 4 5 6 7	1 chunk	+12 lines, -0 lines	0 comments	Download
A	src/opts/SkBlurImage_opts_SSE2.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+115 lines, -0 lines	0 comments	Download
A	src/opts/SkBlurImage_opts_none.cpp	View	1 2 3 4 5 6 7	1 chunk	+14 lines, -0 lines	0 comments	Download
M	src/opts/opts_check_SSE2.cpp	View	1 2 3 4	2 chunks	+14 lines, -0 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

mtklein

Haven't worked through the SSE yet. Will tomorrow! https://codereview.chromium.org/61643011/diff/80001/src/effects/SkBlurImageFilter.cpp File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/61643011/diff/80001/src/effects/SkBlurImageFilter.cpp#newcode192 src/effects/SkBlurImageFilter.cpp:192: SkBoxBlurProc ...

7 years, 1 month ago (2013-11-08 02:11:03 UTC) #2

mtklein

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts_SSE2.h File src/opts/SkBlurImage_opts_SSE2.h (right): https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts_SSE2.h#newcode11 src/opts/SkBlurImage_opts_SSE2.h:11: template <SkBlurDirection srcDirection, SkBlurDirection dstDirection> Okay, this breaks my ...

7 years, 1 month ago (2013-11-08 02:17:59 UTC) #3

Stephen White

Thanks for your review. https://codereview.chromium.org/61643011/diff/80001/src/effects/SkBlurImageFilter.cpp File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/61643011/diff/80001/src/effects/SkBlurImageFilter.cpp#newcode192 src/effects/SkBlurImageFilter.cpp:192: SkBoxBlurProc boxBlurX = SkBoxBlurGetPlatformProc(kX_BlurDirection, kX_BlurDirection); ...

7 years, 1 month ago (2013-11-08 15:47:47 UTC) #4

Thanks for your review.

https://codereview.chromium.org/61643011/diff/80001/src/effects/SkBlurImageFi...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/61643011/diff/80001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:192: SkBoxBlurProc boxBlurX =
SkBoxBlurGetPlatformProc(kX_BlurDirection, kX_BlurDirection);
On 2013/11/08 02:11:03, mtklein wrote:
> This feels weird, like unnecessary degrees of freedom.  Would prefer something
> like
> 
> SkBoxBlurProc boxBlurX, boxBlurY, boxBlurXY;
> if (!SkBoxBlurGetPlatformProcs(&boxBlurX, &boxBlurY, &boxBlurXY)) {
>   const SkBlurDirection X = kX_BlurDirection, Y = kY_BlurDirection;
>   boxBlurX  = boxBlur<X,X>;
>   boxBlurY  = boxBlur<Y,Y>;
>   boxBlurXY = boxBlur<X,Y>;
> }

Done.

https://codereview.chromium.org/61643011/diff/80001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:201: if (!boxBlurY) {
On 2013/11/08 02:11:03, mtklein wrote:
> oh dear.
> 
> -> boxBlurXY?

Thanks! Good catch. Should be obsoleted by new code now.

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts...
File src/opts/SkBlurImage_opts_SSE2.cpp (right):

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_SSE2.cpp:68: SK_PREFETCH(sptr + (rightOffset + 1) *
srcStrideX);
On 2013/11/08 02:11:03, mtklein wrote:
> This reminds me, >=xmmintrin.h has _mm_prefetch, suspiciously similar to
> SK_PREFETCH but more portable (i.e. works on Windows).  I think the right
thing
> to do is actually update SK_PREFETCH to use _mm_prefetch when (SSE && !GCC),
but
> just wanted to mention it here in case you prefer to use it directly.

Cool! Done. (I think it's better to do it explicitly, since we don't compile
skia with SSE2 enabled for all files, so SK_PREFETCH couldn't use it without a
CPUID check).

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts...
File src/opts/SkBlurImage_opts_SSE2.h (right):

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_SSE2.h:11: template <SkBlurDirection srcDirection,
SkBlurDirection dstDirection>
On 2013/11/08 02:17:59, mtklein wrote:
> Okay, this breaks my brain / whole model of templates.  How does this work!? 
Do
> the declarations at the bottom of the .cpp force those versions to be
> instantiated in SkBlurImage_opts_SSE2.o?  What happens if you leave out the
> as-yet-unused YX?

Yes, this is explicit template instantiation. If I left out the instantiation of
YX, it would simply leave out a function for the linker to find. Think about it
in non-templated terms: it would be as if I left out a function definition from
the .cpp: if I left it in, it would be removed from the binary at link time. If
I removed the others, the link would fail on a missing function.

At any rate, now that we've removed the need for the common enum, I've removed
the explicit template instatiation as well.

mtklein

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts_SSE2.cpp File src/opts/SkBlurImage_opts_SSE2.cpp (right): https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts_SSE2.cpp#newcode68 src/opts/SkBlurImage_opts_SSE2.cpp:68: SK_PREFETCH(sptr + (rightOffset + 1) * srcStrideX); On 2013/11/08 ...

7 years, 1 month ago (2013-11-08 18:42:33 UTC) #5

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts...
File src/opts/SkBlurImage_opts_SSE2.cpp (right):

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_SSE2.cpp:68: SK_PREFETCH(sptr + (rightOffset + 1) *
srcStrideX);
On 2013/11/08 15:47:47, Stephen White wrote:
> On 2013/11/08 02:11:03, mtklein wrote:
> > This reminds me, >=xmmintrin.h has _mm_prefetch, suspiciously similar to
> > SK_PREFETCH but more portable (i.e. works on Windows).  I think the right
> thing
> > to do is actually update SK_PREFETCH to use _mm_prefetch when (SSE && !GCC),
> but
> > just wanted to mention it here in case you prefer to use it directly.
> 
> Cool! Done. (I think it's better to do it explicitly, since we don't compile
> skia with SSE2 enabled for all files, so SK_PREFETCH couldn't use it without a
> CPUID check).

Right.  I was thinking rather the other way, that we just #ifdef __SSE__ and
only do _mm_prefetch in SK_PREFETCH when we know we've got SSE globally.

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts...
File src/opts/SkBlurImage_opts_SSE2.h (right):

https://codereview.chromium.org/61643011/diff/80001/src/opts/SkBlurImage_opts...
src/opts/SkBlurImage_opts_SSE2.h:11: template <SkBlurDirection srcDirection,
SkBlurDirection dstDirection>
On 2013/11/08 15:47:47, Stephen White wrote:
> On 2013/11/08 02:17:59, mtklein wrote:
> > Okay, this breaks my brain / whole model of templates.  How does this work!?

> Do
> > the declarations at the bottom of the .cpp force those versions to be
> > instantiated in SkBlurImage_opts_SSE2.o?  What happens if you leave out the
> > as-yet-unused YX?
> 
> Yes, this is explicit template instantiation. If I left out the instantiation
of
> YX, it would simply leave out a function for the linker to find. Think about
it
> in non-templated terms: it would be as if I left out a function definition
from
> the .cpp: if I left it in, it would be removed from the binary at link time.
If
> I removed the others, the link would fail on a missing function.
> 
> At any rate, now that we've removed the need for the common enum, I've removed
> the explicit template instatiation as well.

Thanks, that helps.  I guess what still surprises me is that declaring the
specializations is enough to instantiate them.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
File src/opts/SkBlurImage_opts_SSE2.cpp (right):

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:16: enum BlurDirection {
namespace {} around the enum and template?

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:22: int leftOffset, int rightOffset, int
width, int height)
funky indent

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:24: int rightBorder = SkMin32(rightOffset +
1, width);
in the interest of readability, can you sprinkle const all over this wherever
possible?  there are enough variables to keep track of here that it's not always
obvious what's moving when reading.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:29: __m128i scale = _mm_set1_epi32((1 << 24)
/ kernelSize);
If I'm thinking right, this is the limiting factor in how many pixels we can
handle at a time.  An alternative is to load up two (or four) pixels at once and
use a divide, right?  Any sense of how that'd compare?  I'm guessing, speed
faster but code much grungier?

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:37: expanded = _mm_unpacklo_epi8(expanded,
zero);
Can you narrate a little for folks who don't speak SSE?  E.g, for this block,
something like  // 0000 0000 0000 ARGB  -> 000A 000R 000G 000B would be plenty. 
(Have I got that right?)  This (and its inverse) might make nice little inline
functions.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:46: __m128i tmp1 = _mm_mul_epu32(sum, scale);
So, I'd following along right there with you if this said something like

// (sum * scale + half) >> 24 to do (sum / kernelSize) without a divide.
_m128i average = _mm_srli_epi32(_mm_add_epi32(_mm_mul_epu32(sum, scale), half),
24);

How do tmp2 and the shuffles figure into it?

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:53: result = _mm_packs_epi32(result, zero);
These two lines are // 000A 000R 000G 000B -> 0000 0000 0000 ARGB, right?

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:72: _mm_prefetch(reinterpret_cast<const
char*>(sptr + (rightOffset + 1) * srcStrideX), _MM_HINT_T0);
linebreak after the ,?

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
File src/opts/SkBlurImage_opts_none.cpp (right):

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_none.cpp:12: SkBoxBlurProc* boxBlurXY);
Should be { return false; } ?  Maybe this file's just not needed?

Stephen White

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opts_SSE2.cpp File src/opts/SkBlurImage_opts_SSE2.cpp (right): https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opts_SSE2.cpp#newcode16 src/opts/SkBlurImage_opts_SSE2.cpp:16: enum BlurDirection { On 2013/11/08 18:42:34, mtklein wrote: > ...

7 years, 1 month ago (2013-11-08 20:20:48 UTC) #6

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
File src/opts/SkBlurImage_opts_SSE2.cpp (right):

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:16: enum BlurDirection {
On 2013/11/08 18:42:34, mtklein wrote:
> namespace {} around the enum and template?

Done.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:22: int leftOffset, int rightOffset, int
width, int height)
On 2013/11/08 18:42:34, mtklein wrote:
> funky indent

Fixed.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:24: int rightBorder = SkMin32(rightOffset +
1, width);
On 2013/11/08 18:42:34, mtklein wrote:
> in the interest of readability, can you sprinkle const all over this wherever
> possible?  there are enough variables to keep track of here that it's not
always
> obvious what's moving when reading.

I usually find it's just noise, but ok. Done.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:29: __m128i scale = _mm_set1_epi32((1 << 24)
/ kernelSize);
On 2013/11/08 18:42:34, mtklein wrote:
> If I'm thinking right, this is the limiting factor in how many pixels we can
> handle at a time.  An alternative is to load up two (or four) pixels at once
and
> use a divide, right?  Any sense of how that'd compare?  I'm guessing, speed
> faster but code much grungier?

There is no integer divide in SSE2.

You're right that loading more than a pixel is usually a win (see SkBlitRow_SSE2
where I load 4 src pixels into an aligned SSE reg at a time). The problem here
is that the sums here are inter-pixel, and SSE2 has no horizontal adds. SSSE3 or
SSE4.1 does (can't remember which), but they're still not very fast. We'd also
have to do a lot of tricky setup and teardown code to handle the slop.

We could definitely look at it in a future patch, but I'd prefer to go with this
for now.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:37: expanded = _mm_unpacklo_epi8(expanded,
zero);
On 2013/11/08 18:42:34, mtklein wrote:
> Can you narrate a little for folks who don't speak SSE?  E.g, for this block,
> something like  // 0000 0000 0000 ARGB  -> 000A 000R 000G 000B would be
plenty. 
> (Have I got that right?)  This (and its inverse) might make nice little inline
> functions.

Done.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:46: __m128i tmp1 = _mm_mul_epu32(sum, scale);
On 2013/11/08 18:42:34, mtklein wrote:
> So, I'd following along right there with you if this said something like
> 
> // (sum * scale + half) >> 24 to do (sum / kernelSize) without a divide.
> _m128i average = _mm_srli_epi32(_mm_add_epi32(_mm_mul_epu32(sum, scale),
half),
> 24);
> 
> How do tmp2 and the shuffles figure into it?

Those 7 instructions are an unfortunate side-effect of the fact that SSE2 has no
equivalent of C-style overflowing multiply, only a 32-to-64-bit
(non-overflowing). I'll add a comment.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:53: result = _mm_packs_epi32(result, zero);
On 2013/11/08 18:42:34, mtklein wrote:
> These two lines are // 000A 000R 000G 000B -> 0000 0000 0000 ARGB, right?

Yep, comment added.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:72: _mm_prefetch(reinterpret_cast<const
char*>(sptr + (rightOffset + 1) * srcStrideX), _MM_HINT_T0);
On 2013/11/08 18:42:34, mtklein wrote:
> linebreak after the ,?

Done.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
File src/opts/SkBlurImage_opts_none.cpp (right):

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_none.cpp:12: SkBoxBlurProc* boxBlurXY);
On 2013/11/08 18:42:34, mtklein wrote:
> Should be { return false; } ?  Maybe this file's just not needed?

Yikes, thanks! Yes, it's needed. (I did check that Android built, but it does
lazy-loading, so it doesn't fail until runtime.)

mtklein

Just a couple nits. LGTM. https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opts_SSE2.cpp File src/opts/SkBlurImage_opts_SSE2.cpp (right): https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opts_SSE2.cpp#newcode29 src/opts/SkBlurImage_opts_SSE2.cpp:29: __m128i scale = _mm_set1_epi32((1 ...

7 years, 1 month ago (2013-11-08 20:37:34 UTC) #7

Just a couple nits.  LGTM.

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
File src/opts/SkBlurImage_opts_SSE2.cpp (right):

https://codereview.chromium.org/61643011/diff/210001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:29: __m128i scale = _mm_set1_epi32((1 << 24)
/ kernelSize);
On 2013/11/08 20:20:49, Stephen White wrote:
> On 2013/11/08 18:42:34, mtklein wrote:
> > If I'm thinking right, this is the limiting factor in how many pixels we can
> > handle at a time.  An alternative is to load up two (or four) pixels at once
> and
> > use a divide, right?  Any sense of how that'd compare?  I'm guessing, speed
> > faster but code much grungier?
> 
> There is no integer divide in SSE2.
> 
> You're right that loading more than a pixel is usually a win (see
SkBlitRow_SSE2
> where I load 4 src pixels into an aligned SSE reg at a time). The problem here
> is that the sums here are inter-pixel, and SSE2 has no horizontal adds. SSSE3
or
> SSE4.1 does (can't remember which), but they're still not very fast. We'd also
> have to do a lot of tricky setup and teardown code to handle the slop.
> 
> We could definitely look at it in a future patch, but I'd prefer to go with
this
> for now.

Duh.  SGTM

https://codereview.chromium.org/61643011/diff/330001/src/opts/SkBlurImage_opt...
File src/opts/SkBlurImage_opts_SSE2.cpp (right):

https://codereview.chromium.org/61643011/diff/330001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:27: inline __m128i expand(int a) {
int -> uint32_t?

https://codereview.chromium.org/61643011/diff/330001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:33: // 0 0 0 0   0 0 0 0   0 A 0 R   G 0 B 0
It's 0 G 0 B, right?

https://codereview.chromium.org/61643011/diff/330001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:106: };
I just know something's going to yell at you for a stray ;

Better to end with // namespace

:)

Stephen White

https://codereview.chromium.org/61643011/diff/330001/src/opts/SkBlurImage_opts_SSE2.cpp File src/opts/SkBlurImage_opts_SSE2.cpp (right): https://codereview.chromium.org/61643011/diff/330001/src/opts/SkBlurImage_opts_SSE2.cpp#newcode27 src/opts/SkBlurImage_opts_SSE2.cpp:27: inline __m128i expand(int a) { On 2013/11/08 20:37:35, mtklein ...

7 years, 1 month ago (2013-11-08 20:47:44 UTC) #8

Stephen White

Committed patchset #10 manually as r12204 (presubmit successful).

7 years, 1 month ago (2013-11-08 20:49:14 UTC) #9

mtklein

7 years, 1 month ago (2013-11-08 20:49:21 UTC) #10

Message was sent while issue was closed.

lgtm

https://codereview.chromium.org/61643011/diff/330001/src/opts/SkBlurImage_opt...
File src/opts/SkBlurImage_opts_SSE2.cpp (right):

https://codereview.chromium.org/61643011/diff/330001/src/opts/SkBlurImage_opt...
src/opts/SkBlurImage_opts_SSE2.cpp:27: inline __m128i expand(int a) {
On 2013/11/08 20:47:45, Stephen White wrote:
> On 2013/11/08 20:37:35, mtklein wrote:
> > int -> uint32_t?
> 
> The declaration for _mm_cvtsi32_si128 just takes an int, so I'd rather just
> stick with that for compatibility.

SGTM

Expand Messages | Collapse Messages