ARM Skia NEON patches - 41 - arm64: SkXfermode::xfer32

ARM Skia NEON patches - 41 - arm64: SkXfermode::xfer32

Currently the NEON code for Xfermodes performs well on arm64
targets except for dstout and dstin which are significantly
slower than the C code. This patch fixes this and gives
further improvements on other modes.

Here are some perf results:

+------------+------------+------------+
| mode       | Cortex-A53 | Cortex-A57 |
+------------+------------+------------+
| multiply   |    +24.58% |    +23.71% |
+------------+------------+------------+
| exclusion  |    +22.72% |    +22.05% |
+------------+------------+------------+
| difference |    +34.67% |    +36.82% |
+------------+------------+------------+
| hardlight  |    +17.07% |    +14.74% |
+------------+------------+------------+
| lighten    |    +38.21% |    +32.87% |
+------------+------------+------------+
| darken     |    +37.59% |    +32.99% |
+------------+------------+------------+
| overlay    |    +17.36% |    +16.88% |
+------------+------------+------------+
| screen     |    +52.56% |    +54.43% |
+------------+------------+------------+
| modulate   |    +62.85% |    +61.32% |
+------------+------------+------------+
| plus       |    +91.52% |   +117.41% |
+------------+------------+------------+
| xor        |    +42.86% |    +43.38% |
+------------+------------+------------+
| dstatop    |    +48.46% |    +48.99% |
+------------+------------+------------+
| srcatop    |    +50.50% |    +48.51% |
+------------+------------+------------+
| dstout     |    +67.83% |    +78.09% |
+------------+------------+------------+
| srcout     |    +69.02% |    +78.26% |
+------------+------------+------------+
| dstin      |    +70.92% |    +79.24% |
+------------+------------+------------+
| srcin      |    +68.90% |    +78.23% |
+------------+------------+------------+
| dstover    |    +73.80% |    +68.10% |
+------------+------------+------------+

Signed-off-by: Kévin PETIT <kevin.petit@arm.com>;

BUG=skia

Committed: https://skia.googlesource.com/skia/+/4dc94d9dfc62ea22649952fbaa1fdb95bab61d7a

[kevin.petit, 2014-06-30 13:12:13]

ping.

[mtklein, 2014-06-30 13:28:03]

https://codereview.chromium.org/350343002/diff/1/src/opts/SkXfermode_opts_arm...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/350343002/diff/1/src/opts/SkXfermode_opts_arm...
src/opts/SkXfermode_opts_arm_neon.cpp:807: asm volatile (
Can you take me through this and remind my why it can't be intrinsics
everywhere?  Seems like we only want something super simple:

vst4_8(dst, procSIMD(vld4_8(src), vld4_8(dst)));
src += 8;
dst += 8;

is that wrong, unsupported, or just not fast enough?  Are we getting the win
here from the post-increments?  Is there really no way to get GCC to generate
post-increments?

[kevin.petit, 2014-06-30 13:38:05]

https://codereview.chromium.org/350343002/diff/1/src/opts/SkXfermode_opts_arm...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/350343002/diff/1/src/opts/SkXfermode_opts_arm...
src/opts/SkXfermode_opts_arm_neon.cpp:807: asm volatile (
On 2014/06/30 13:28:03, mtklein wrote:
> Can you take me through this and remind my why it can't be intrinsics
> everywhere?  Seems like we only want something super simple:
> 
> vst4_8(dst, procSIMD(vld4_8(src), vld4_8(dst)));
> src += 8;
> dst += 8;

Correct.

> is that wrong, unsupported, or just not fast enough?

It's _really_ slower than what it should be. Try to build with the intrinsics
and have a look at the assembly if you're feeling brave enough. Various
toolchains are broken in various ways and all that assembly is only here to work
around the issues.

> Are we getting the win
> here from the post-increments?  Is there really no way to get GCC to generate
> post-increments?

We do gain from the post-increments but in a lot of cases, aka when it's not
broken, gcc is able to use them.

[mtklein, 2014-06-30 13:44:35]

lgtm

Do you figure the indirection through the proc pointer is also hurting us here?

https://codereview.chromium.org/350343002/diff/1/src/opts/SkXfermode_opts_arm...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/350343002/diff/1/src/opts/SkXfermode_opts_arm...
src/opts/SkXfermode_opts_arm_neon.cpp:762: while (count >= 8) {
Do you think you could tack on some note like this for me for next time?

// We'd like to just do this:
//   vst4_8(dst, procSIMD(vld4_8(src), vld4_8(dst)));
//   src += 8;
//   dst += 8;
// but that tends to generate miserable code.  Here are a bunch of faster
// workarounds for different architectures and compilers.

[kevin.petit, 2014-06-30 14:04:40]

>Do you figure the indirection through the proc pointer is also hurting us here?

I remember I tried without it and it didn't make a big difference (on v7). Also
not having the indirection would mean having one class per transfer mode. That
could probably be written in terms of templates but to be honest, I haven't gone
that far in experiments.

https://codereview.chromium.org/350343002/diff/1/src/opts/SkXfermode_opts_arm...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/350343002/diff/1/src/opts/SkXfermode_opts_arm...
src/opts/SkXfermode_opts_arm_neon.cpp:762: while (count >= 8) {
On 2014/06/30 13:44:35, mtklein wrote:
> Do you think you could tack on some note like this for me for next time?
> 
> // We'd like to just do this:
> //   vst4_8(dst, procSIMD(vld4_8(src), vld4_8(dst)));
> //   src += 8;
> //   dst += 8;
> // but that tends to generate miserable code.  Here are a bunch of faster
> // workarounds for different architectures and compilers.

Done.

[kevin.petit, 2014-06-30 14:05:15]

The CQ bit was checked by kevin.petit@arm.com

[commit-bot: I haz the power, 2014-06-30 14:05:48]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kevin.petit@arm.com/350343002/20001

[commit-bot: I haz the power, 2014-06-30 14:32:26]

Change committed as 4dc94d9dfc62ea22649952fbaa1fdb95bab61d7a

[Tom Hudson, 2014-12-02 16:24:27]

tomhudson@chromium.org changed reviewers:
+ tomhudson@chromium.org

[Tom Hudson, 2014-12-02 16:24:27]

ARM: can you revisit?

This CL has been implicated in a series of crashes on the new Nexus 9 and so is
in danger of being reverted for Android.
Ben Murdoch in our London office has a reproduction case (cc'd). It *may* only
be on prototype hardware.

[kevin.petit, 2014-12-02 17:16:24]

Hi Tom,

On 2014/12/02 16:24:27, Tom Hudson wrote:
> ARM: can you revisit?

Sure, I'll have a look.

> This CL has been implicated in a series of crashes on the new Nexus 9 and so
is
> in danger of being reverted for Android.

I'd rather avoid that :-).

> Ben Murdoch in our London office has a reproduction case (cc'd).

Ben, could you send me the reproducer please?

Regards,
Kévin