Optimized premultiplying swizzles for NEON

src/opts/SkSwizzler_opts.h

Index: src/opts/SkSwizzler_opts.h
diff --git a/src/opts/SkSwizzler_opts.h b/src/opts/SkSwizzler_opts.h
new file mode 100644
index 0000000000000000000000000000000000000000..bb3575c24c2c6642763786621cc63677c0d8e24b
--- /dev/null
+++ b/src/opts/SkSwizzler_opts.h
@@ -0,0 +1,134 @@
+/*
+ * 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_opts_DEFINED
+#define SkSwizzler_opts_DEFINED
+
+#include "SkColorPriv.h"
+
+namespace SK_OPTS_NS {
+
+// These variable names in these functions just pretend the input is RGBA.

[mtklein, 2016/01/13 17:04:37]

You're going to the dark side!

BGRA is the format to prefer, because it matches SkColor.  Written as a literal,
it's 0xAARRGGBB, which is what we're used to writing and breaking apart with
shifts.

I admit this is pretty arbitrary, but the arbitrariness is why I'd like to stick
to the convention of writing things as if they were BGRA.  It's nice to hop back
and forth between files without twisting your brain.

[msarett, 2016/01/13 18:52:05]

SGTM.  I just wanted NEON and default to be the same.  Didn't know it mattered
which one I picked :).

+// They work fine with both RGBA and BGRA.
+
+#if defined(SK_ARM_HAS_NEON)
+
+// Rounded divide by 255 (x + 127) / 255

[mtklein, 2016/01/13 17:04:36]

add a comma after the first 255?

[msarett, 2016/01/13 18:52:05]

Done.

+//    result = (x + 127) / 255

[mtklein, 2016/01/13 17:04:36]

Let's move the rest of these implementation-focused comments to just inside the
function body.

[msarett, 2016/01/13 18:52:05]

Done.

+//    result = (x + 127) / 256 + error1
+//
+//    error1 = (x + 127) / (255 * 256)
+//    error1 = (x + 127) / (256 * 256) + error2
+//
+//    error2 = (x + 127) / (255 * 256 * 256)
+//
+//    The maximum value of error2 is too small to matter.  Thus:
+//    result = (x + 127) / 256 + (x + 127) / (256 * 256)
+//    result = ((x + 127) / 256 + x + 127) / 256
+//    result = ((x + 127) >> 8 + x + 127) >> 8
+//
+//    Use >>> to represent "rounded right shift" which, conveniently,
+//    NEON supports.

[mtklein, 2016/01/13 17:04:37]

...supports in one instruction.
?

[msarett, 2016/01/13 18:52:05]

Done.

+//    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.
+static uint8x8_t div255_round(uint16x8_t x) {
+    return vraddhn_u16(x, vrshrq_n_u16(x, 8));
+}
+
+// Scale a byte by another (x * y + 127) / 255

[mtklein, 2016/01/13 17:04:37]

comma after another?

[msarett, 2016/01/13 18:52:05]

Done.

+static uint8x8_t scale(uint8x8_t x, uint8x8_t y) {
+    return div255_round(vmull_u8(x, y));
+}
+
+template <bool kSwapRB>
+static void premul_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;

[mtklein, 2016/01/13 17:04:36]

Generally I prefer to cast the pointers inside each load and store, so that all
the constants we use match up.  It takes thought to see that src8 += 32 is
correct, but while src += 8 is a bit clearer.

[msarett, 2016/01/13 18:52:05]

Done.

+    for (; i < count; i += 8) {

[mtklein, 2016/01/13 17:04:36]

I think this walks i too far for, e.g. count == 15.

I've found the safest way to get these things right is to decrement count:

while (count >= 8) {

    count -= 8;
}
while (count > 0) {

    count--;
}

I  further usually write that last one as 

while (count --> 0) {

}

because it both looks cute and is a pretty strong indication that count will not
be referred to inside the loop.

[msarett, 2016/01/13 18:52:05]

Done.  Apologies for the messy job porting this code.

I've verified that perf is unaffected and tested again (this time with the opts
turned on :)).

+        // Load 8 pixels.
+        uint8x8x4_t rgba = vld4_u8(src8);
+
+        uint8x8_t r = rgba.val[0],
+                  g = rgba.val[1],
+                  b = rgba.val[2],
+                  a = rgba.val[3];
+
+        // Premultiply.
+        r = scale(r, a);
+        g = scale(g, a);
+        b = scale(b, a);
+
+        // Store 8 premultiplied pixels.
+        if (kSwapRB) {
+            rgba.val[0] = b;
+            rgba.val[1] = g;
+            rgba.val[2] = r;
+        } else {
+            rgba.val[0] = r;
+            rgba.val[1] = g;
+            rgba.val[2] = b;
+        }
+        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]);

[mtklein, 2016/01/13 17:04:37]

dst[i] = ...
...
dst++

this steps dst forward twice.

I've found the most foolproof way to write these methods never needs an 'i':
decrement count, increment dst and src.

[msarett, 2016/01/13 18:52:05]

Done.

+        src8 += 4;
+        dst++;
+    }
+}
+
+#else
+
+template <bool kSwapRB>
+static void premul_xxxa(uint32_t dst[], const uint32_t src[], int count) {

[mtklein, 2016/01/13 17:04:37]

This change is fine by me if you like, but it's not necessary once you add
shims.

[msarett, 2016/01/13 18:52:05]

Acknowledged.  Changing back to the original.

+    for (int i = 0; i < count; i++) {
+        uint8_t a = src[i] >> 24,
+                b = src[i] >> 16,
+                g = src[i] >>  8,
+                r = src[i] >>  0;
+        b = (b*a+127)/255;
+        g = (g*a+127)/255;
+        r = (r*a+127)/255;
+        if (kSwapRB) {
+            dst[i] = (uint32_t)a << 24
+                   | (uint32_t)b << 16
+                   | (uint32_t)g <<  8
+                   | (uint32_t)r <<  0;
+        } else {
+            dst[i] = (uint32_t)a << 24
+                   | (uint32_t)r << 16
+                   | (uint32_t)g <<  8
+                   | (uint32_t)b <<  0;
+        }
+    }
+}
+
+#endif
+
+static void swaprb_xxxa(uint32_t dst[], const uint32_t src[], int count) {
+    for (int i = 0; i < count; i++) {
+        uint8_t a = src[i] >> 24,
+                b = src[i] >> 16,
+                g = src[i] >>  8,
+                r = src[i] >>  0;
+        dst[i] = (uint32_t)a << 24
+               | (uint32_t)r << 16
+               | (uint32_t)g <<  8
+               | (uint32_t)b <<  0;
+    }
+}
+
+}
+
+#endif // SkSwizzler_opts_DEFINED