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

src/opts/SkBlurImage_opts_neon.cpp

Index: src/opts/SkBlurImage_opts_neon.cpp
diff --git a/src/opts/SkBlurImage_opts_neon.cpp b/src/opts/SkBlurImage_opts_neon.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..d1985c09cc6da8201f5d73c3929d0902b2b3cf8d
--- /dev/null
+++ b/src/opts/SkBlurImage_opts_neon.cpp
@@ -0,0 +1,98 @@
+/*
+ * Copyright 2013 The Android Open Source Project
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+
+#include "SkBitmap.h"
+#include "SkColorPriv.h"
+#include "SkBlurImage_opts.h"
+#include "SkRect.h"
+
+#include <arm_neon.h>
+
+namespace {
+
+enum BlurDirection {
+    kX, kY
+};
+
+/**
+ * Helper function to spread the components of a 32-bit integer into the
+ * lower 8 bits of each 32-bit element of an SSE register.
+ */
+
+inline uint32x4_t expand(uint32_t a) {
+    uint8x8_t v8 = vreinterpret_u8_u32(vdup_n_u32(a));

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

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?

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

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.
The 4 sums that this is used in all have to be 32bit anyway, so we need to
promote it regardless.

+    const uint16x4_t v16 = vget_low_u16(vmovl_u8(v8));
+    return vmovl_u16(v16);
+}
+
+template<BlurDirection srcDirection, BlurDirection dstDirection>
+void SkBoxBlur_SSE2(const SkPMColor* src, int srcStride, SkPMColor* dst, int kernelSize,

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

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).

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

Whoops, busted. :) Fixed.
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.

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

SGTM

+                    int leftOffset, int rightOffset, int width, int height)
+{
+    const int rightBorder = SkMin32(rightOffset + 1, width);
+    const int srcStrideX = srcDirection == kX ? 1 : srcStride;
+    const int dstStrideX = dstDirection == kX ? 1 : height;
+    const int srcStrideY = srcDirection == kX ? srcStride : 1;
+    const int dstStrideY = dstDirection == kX ? width : 1;
+    const uint32x4_t scale = vdupq_n_u32((1 << 24) / kernelSize);
+    const uint32x4_t half = vdupq_n_u32(1 << 23);
+    for (int y = 0; y < height; ++y) {
+        uint32x4_t sum = vdupq_n_u32(0);
+        const SkPMColor* p = src;
+        for (int i = 0; i < rightBorder; ++i) {
+            sum = vaddq_u32(sum, expand(*p));
+            p += srcStrideX;
+        }
+
+        const SkPMColor* sptr = src;
+        SkColor* dptr = dst;
+        for (int x = 0; x < width; ++x) {
+            // half+sumA*scale, half+sumR*scale, half+sumG*scale, half+sumB*scale
+            uint32x4_t result = vmlaq_u32(half, sum, scale);
+
+            // Shift down to lower 8 bits of each element.
+            result = vshrq_n_u32(result, 24);
+
+            //   A   R   G   B
+            uint16x4_t result16 = vqmovn_u32(result);
+
+            // A R G B A R G B
+            uint8x8_t result8 = vqmovn_u16(vcombine_u16(result16, result16));
+
+            vst1_lane_u32(dptr, vreinterpret_u32_u8(result8), 0);
+            if (x >= leftOffset) {
+                const SkPMColor* l = sptr - leftOffset * srcStrideX;

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

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?

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

Thanks, done.

+                sum = vsubq_u32(sum, expand(*l));
+            }
+            if (x + rightOffset + 1 < width) {
+                const SkPMColor* r = sptr + (rightOffset + 1) * srcStrideX;
+                sum = vaddq_u32(sum, expand(*r));
+            }
+            sptr += srcStrideX;
+            if (srcDirection == kY) {
+                SK_PREFETCH(sptr + (rightOffset + 1) * srcStrideX);
+            }
+            dptr += dstStrideX;
+        }
+        src += srcStrideY;
+        dst += dstStrideY;
+    }
+}
+
+} // namespace
+
+bool SkBoxBlurGetPlatformProcs(SkBoxBlurProc* boxBlurX,
+                               SkBoxBlurProc* boxBlurY,
+                               SkBoxBlurProc* boxBlurXY,
+                               SkBoxBlurProc* boxBlurYX) {
+    *boxBlurX = SkBoxBlur_SSE2<kX, kX>;

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

_SSE2 here too

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

Fixed.

+    *boxBlurY = SkBoxBlur_SSE2<kY, kY>;
+    *boxBlurXY = SkBoxBlur_SSE2<kX, kY>;
+    *boxBlurYX = SkBoxBlur_SSE2<kY, kX>;
+    return true;
+}