Optimized premultiplying swizzles for NEON

src/opts/SkSwizzler_neon.h

+/*
+ * Copyright 2016 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef SkSwizzler_neon_DEFINED
+#define SkSwizzler_neon_DEFINED
+
+#include "SkColorPriv.h"
+
+#include <arm_neon.h>
+
+namespace sk_neon {
+
+static void premul_xxxa(uint32_t dst[], const uint32_t src[], int count) {

[mtklein, 2016/01/13 15:20:01]

I think some of the clarity of this code is getting swamped by the comments.

Going to mark a few of them up with suggestions.

[msarett, 2016/01/13 16:17:15]

Acknowledged.

+    int i = 0;
+    uint8_t* dst8 = (uint8_t*) dst;
+    const uint8_t* src8 = (const uint8_t*) src;
+    for (; i < count; i += 8) {
+        // Load 8 pixels.  This load instruction will deinterleave the pixels,
+        // producing alphas, reds, greens, and blues each in its own vector.
+        uint8x8x4_t rgba = vld4_u8(src8);

[mtklein, 2016/01/13 15:20:01]

Load 8 pixels.

[msarett, 2016/01/13 16:17:15]

Done.

+
+        // These should be free operations.

[mtklein, 2016/01/13 15:20:01]

I don't think you need any comment here.  I'd suggest

uint8x8_t r = rgba.val[0],
          g = rgba.val[1],
          b = rgba.val[2],
          a = rgba.val[3];

We all know what r,g,b,a are, and this keeps them short and equal-length.

[msarett, 2016/01/13 16:17:15]

Done.

+        uint8x8_t reds = rgba.val[0];
+        uint8x8_t greens = rgba.val[1];
+        uint8x8_t blues = rgba.val[2];
+        uint8x8_t alphas = rgba.val[3];
+
+        // Premultiplication Step 1: Multiply each color component by the
+        // alpha component.  Note that this also widens each of the components
+        // to 16-bits.
+        uint16x8_t product_reds = vmull_u8(reds, alphas);

[mtklein, 2016/01/13 15:20:01]

I'd suggest:

// Premultiply.
r = scale(r, a);
g = scale(g, a);
b = scale(b, a);

where

// (x*y+127)/255, i.e. scale a byte by another.
static uint8x8_t scale(uint8x8_t x, uint8x8_t y) { ... }

possibly using this as a sub-routine:

// (x+127)/255.
static uint8x8_t div255_round(uint16x8_t x) { ... }

We probably don't need to prove that div255() does div255(), as our tests and
Gold will do that, but if you want to prove it here, let's squirrel it away
inside a div255_round() method so we don't have to keep reading it every time we
read this code.

[msarett, 2016/01/13 16:17:16]

I think the refactoring makes things clearer.

My preference is to keep the comments.  I agree that it is cluttered as is, but
it's be nice if someone other than you or me can understand what's going on. 
(And also I may forget that proof in year.)  Gold will verify that it works, but
I'd also like to explain why it works.

+        uint16x8_t product_greens = vmull_u8(greens, alphas);
+        uint16x8_t product_blues = vmull_u8(blues, alphas);
+
+        // Premultiplication Step 2: Perform a rounded divide by 255.
+        //    result = (x + 128) / 255
+        //    result = (x + 128) / 256 + error1
+        //
+        //    error1 = (x + 128) / (255 * 256)
+        //    error1 = (x + 128) / (256 * 256) + error2
+        //
+        //    error2 = (x + 128) / (255 * 256 * 256)
+        //
+        //    The maximum value of error2 is too small to matter.  Thus:
+        //    result = (x + 128) / 256 + (x + 128) / (256 * 256)
+        //    result = ((x + 128) / 256 + x + 128) / 256
+        //    result = ((x + 128) >> 8 + x + 128) >> 8
+        //
+        //    Use >>> to represent "rounded right shift" which, conveniently,
+        //    NEON supports.
+        //    result = ((x >>> 8) + x) >>> 8
+        //
+        //    Note that the second right shift is actually performed as an
+        //    "add, round, and narrow back to 8-bits" instruction.
+        uint8x8_t result_reds = vraddhn_u16(product_reds, vrshrq_n_u16(product_reds, 8));
+        uint8x8_t result_greens = vraddhn_u16(product_greens, vrshrq_n_u16(product_greens, 8));
+        uint8x8_t result_blues = vraddhn_u16(product_blues, vrshrq_n_u16(product_blues, 8));
+
+        // Store 8 premultiplied pixels.

[mtklein, 2016/01/13 15:20:01]

Good from here on.

[msarett, 2016/01/13 16:17:15]

Acknowledged.

+        rgba.val[0] = result_reds;
+        rgba.val[1] = result_greens;
+        rgba.val[2] = result_blues;
+        vst4_u8(dst8, rgba);
+        src8 += 32;
+        dst8 += 32;
+    }
+
+    dst = (uint32_t*) dst8;
+    for (; i < count; i++) {
+        dst[i] = SkPremultiplyARGBInline(src8[3], src8[0], src8[1], src8[2]);
+        src8 += 4;
+        dst++;
+    }
+}
+
+static void premul_swaprb_xxxa(uint32_t dst[], const uint32_t src[], int count) {
+    int i = 0;
+    uint8_t* dst8 = (uint8_t*) dst;
+    const uint8_t* src8 = (const uint8_t*) src;
+    for (; i < count; i += 8) {
+        // Load 8 pixels.  This load instruction will deinterleave the pixels,
+        // producing alphas, reds, greens, and blues each in its own vector.
+        uint8x8x4_t rgba = vld4_u8(src8);
+
+        // These should be free operations.
+        uint8x8_t reds = rgba.val[0];
+        uint8x8_t greens = rgba.val[1];
+        uint8x8_t blues = rgba.val[2];
+        uint8x8_t alphas = rgba.val[3];
+
+        // Premultiplication Step 1: Multiply each color component by the
+        // alpha component.  Note that this also widens each of the components
+        // to 16-bits.
+        uint16x8_t product_reds = vmull_u8(reds, alphas);
+        uint16x8_t product_greens = vmull_u8(greens, alphas);
+        uint16x8_t product_blues = vmull_u8(blues, alphas);
+
+        // Premultiplication Step 2: Perform a rounded divide by 255.
+        //    result = (x + 128) / 255
+        //    result = (x + 128) / 256 + error1
+        //
+        //    error1 = (x + 128) / (255 * 256)
+        //    error1 = (x + 128) / (256 * 256) + error2
+        //
+        //    error2 = (x + 128) / (255 * 256 * 256)
+        //
+        //    The maximum value of error2 is too small to matter.  Thus:
+        //    result = (x + 128) / 256 + (x + 128) / (256 * 256)
+        //    result = ((x + 128) / 256 + x + 128) / 256
+        //    result = ((x + 128) >> 8 + x + 128) >> 8
+        //
+        //    Use >>> to represent "rounded right shift" which, conveniently,
+        //    NEON supports.
+        //    result = ((x >>> 8) + x) >>> 8
+        //
+        //    Note that the second right shift is actually performed as an
+        //    "add, round, and narrow back to 8-bits" instruction.
+        uint8x8_t result_reds = vraddhn_u16(product_reds, vrshrq_n_u16(product_reds, 8));
+        uint8x8_t result_greens = vraddhn_u16(product_greens, vrshrq_n_u16(product_greens, 8));
+        uint8x8_t result_blues = vraddhn_u16(product_blues, vrshrq_n_u16(product_blues, 8));
+
+        // Store 8 premultiplied pixels.
+        rgba.val[0] = result_blues;

[msarett, 2016/01/13 14:05:38]

This is identical to premul_xxxa with the exception of the swap of blue and red
here.  Can we somehow share code?

[mtklein, 2016/01/13 14:33:17]

Yes, template <bool kSwapRB> on the function, then if (kSwapRB) { ... }.

[msarett, 2016/01/13 16:17:16]

Done.

+        rgba.val[1] = result_greens;
+        rgba.val[2] = result_red;
+        vst4_u8(dst8, rgba);
+        src8 += 32;
+        dst8 += 32;
+    }
+
+    dst = (uint32_t*) dst8;
+    for (; i < count; i++) {
+        dst[i] = SkPremultiplyARGBInline(src8[3], src8[0], src8[1], src8[2]);
+        src8 += 4;
+        dst++;
+    }
+}
+
+}
+
+#endif // SkSwizzler_neon_DEFINED