NEON fast path for box blur

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
index 4e33d72d462562284c8feebe9cb1189ca851e52f..8e71b0bcdc2b40f9d8c87cfd6912501fd82f4e89 100644
--- a/src/opts/SkBlurImage_opts_neon.cpp
+++ b/src/opts/SkBlurImage_opts_neon.cpp
@@ -20,6 +20,90 @@ enum BlurDirection {
 };
 
 /**
+ * Helper function to load 2 pixels from diffent rows to a 8x8 NEON register
+ * and also pre-load pixels for future read
+ */
+template<BlurDirection srcDirection>
+inline uint8x8_t load2pixels(const SkPMColor* src, int srcStride) {

[Stephen White, 2013/12/12 05:30:30]

should be separated by underscores; see
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...,
so load_2_pixels()

[zheng.xu, 2013/12/12 08:15:37]

Done.

+    if (srcDirection == kX) {
+        uint32x2_t temp = vdup_n_u32(0);
+        SK_PREFETCH(src + 16);

[Stephen White, 2013/12/12 05:30:30]

Are you sure this offset is correct? src is SkPMColor*, so unless I've done my
pointer math wrong, I think this would be prefetching 64 bytes ahead. Is that
what you had in mind?

At any rate, I think we should check that the prefetches are actually doing
anything in either X or Y, and skip them if they aren't. There are X-only and
Y-only benches which should show the effect.

[zheng.xu, 2013/12/12 08:15:37]

Yes, I actually mean 64 bytes ahead, that's 8 loops. Because it need time to let
the cache system to pre-fetch data in parallel.

I've done some tests locally. 10% faster by adding 2 prefetchs in (srcDirection
== kX). adding prefetch in the else branch doesn't affect the bench data

I left the pre-fetch in (srcDirection == kY), only for keep the symmetry. Do you
mean we'd better remove them?

+        SK_PREFETCH(src + srcStride + 16);
+        return vreinterpret_u8_u32(vld1_lane_u32(src + srcStride, vld1_lane_u32(src, temp, 0), 1));
+    } else {
+        SK_PREFETCH(src + srcStride);
+        return vld1_u8((uint8_t*)src);
+    }
+}
+
+/**
+ * Helper function to store the low 8-bits from a 16x8 NEON register to 2 rows
+ */
+template<BlurDirection dstDirection>
+inline void store2pixels(uint16x8_t result16x8, SkPMColor* dst, int dstStride) {

[Stephen White, 2013/12/12 05:30:30]

store_2_pixels()

[zheng.xu, 2013/12/12 08:15:37]

Done.

+    if (dstDirection == kX) {
+        uint32x2_t temp = vreinterpret_u32_u8(vmovn_u16(result16x8));
+        vst1_lane_u32(dst, temp, 0);
+        vst1_lane_u32(dst + dstStride, temp, 1);
+    } else {
+        uint8x8_t temp = vmovn_u16(result16x8);
+        vst1_u8((uint8_t*)dst, temp);
+    }
+}
+
+/**
+ * fast path for kernel size less than 128
+ */
+template<BlurDirection srcDirection, BlurDirection dstDirection>
+void SkFastBoxBlur_NEON(const SkPMColor* src, int srcStride, SkPMColor* dst, int kernelSize,

[Stephen White, 2013/12/12 05:30:30]

I'd prefer to avoid names like "fast" (since we often come up with faster
algorithms later). Let's name it by what it does, perhaps
SkDoubleRowBoxBlur_NEON.

[zheng.xu, 2013/12/12 08:15:37]

Done.

+                        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 uint16x8_t scale = vdupq_n_u16((1 << 15) / kernelSize);
+
+    int x, y;
+    height &= ~1;

[Stephen White, 2013/12/12 05:30:30]

Let's just pass in pointers to src, dst and height. Then the loop can become:

for (; *height > 0; *height -= 2)
...

and we can omit the &= -1 here and below.

[zheng.xu, 2013/12/12 08:15:37]

Done.

+    for (y = 0; y < height; y += 2) {
+        uint16x8_t sum = vdupq_n_u16(0);
+        uint16x8_t val16x8;
+        uint8x8_t val8x8;
+        const SkPMColor* sptr = src;
+        for (x = 0; x < rightBorder; x++) {
+            val8x8 = load2pixels<srcDirection>(sptr, srcStride);
+            sum = vaddw_u8(sum, val8x8);
+            sptr += srcStrideX;
+        }
+
+        sptr = src;
+        SkPMColor* dptr = dst;
+        for (x = 0; x < width; x++) {
+            // val = (sum * scale * 2 + 0x8000) >> 16
+            val16x8 = vreinterpretq_u16_s16(vqrdmulhq_s16(

[Stephen White, 2013/12/12 05:30:30]

Please declare this variable here where it's used, not up top. And since you'll
see the type in context that way, I wouldn't bother naming it with the type.
E.g.,

uint16x8_t result = ...;
store2pixels<dstDirection>(result, ...);

[zheng.xu, 2013/12/12 08:15:37]

Done.

+                vreinterpretq_s16_u16(sum), vreinterpretq_s16_u16(scale)));
+            store2pixels<dstDirection>(val16x8, dptr, width);
+
+            if (x >= leftOffset) {
+                val8x8 = load2pixels<srcDirection>(sptr - leftOffset * srcStrideX, srcStride);

[Stephen White, 2013/12/12 05:30:30]

Same here.

[zheng.xu, 2013/12/12 08:15:37]

Done.

+                sum = vsubw_u8(sum, val8x8);
+            }
+            if (x + rightOffset + 1 < width) {
+                val8x8 = load2pixels<srcDirection>(sptr + (rightOffset + 1) * srcStrideX, srcStride);

[Stephen White, 2013/12/12 05:30:30]

Same here.

[zheng.xu, 2013/12/12 08:15:37]

Done.

+                sum = vaddw_u8(sum, val8x8);
+            }
+            sptr += srcStrideX;
+            dptr += dstStrideX;
+        }
+        src += srcStrideY * 2;
+        dst += dstStrideY * 2;
+    }
+}
+
+
+/**
  * Helper function to spread the components of a 32-bit integer into the
  * lower 8 bits of each 16-bit element of a NEON register.
  */
@@ -42,7 +126,18 @@ void SkBoxBlur_NEON(const SkPMColor* src, int srcStride, SkPMColor* dst, int ker
     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) {
+
+    int fastHeight = 0;
+    if (kernelSize < 128)

[Stephen White, 2013/12/12 05:30:30]

You should add a GM and a bench for the old case, so that it still gets
exercised (kernel size exactly 128, say).

[zheng.xu, 2013/12/12 08:15:37]

I think there is no API can set the box kernel size. box kernel size is nearly 2
times sigma. We can make a blur case with sigma 80.

+    {
+        SkFastBoxBlur_NEON<srcDirection, dstDirection>(src, srcStride, dst, kernelSize,
+            leftOffset, rightOffset, width, height);

[Stephen White, 2013/12/12 05:30:30]

See above: if we pass in &src &dst and &height here, we don't need to repeat the
computations below.

[zheng.xu, 2013/12/12 08:15:37]

Done.

+        fastHeight = height & (~1);
+        src += srcStrideY * fastHeight;
+        dst += dstStrideY * fastHeight;
+    }
+
+    for (int y = fastHeight; y < height; ++y) {
         uint32x4_t sum = vdupq_n_u32(0);
         const SkPMColor* p = src;
         for (int i = 0; i < rightBorder; ++i) {