Replace NEON assembly memset16 and memset32 with intrinsic versions.

Replace NEON assembly memset16 and memset32 with intrinsic versions.

According to bench/MemsetBench.cpp, I've got them somewhere between 10% slower
and a percent or two faster than the old assembly.

BUG=skia:

CQ_EXTRA_TRYBOTS=client.skia.android:Test-Android-GCC-Nexus5-CPU-NEON-Arm7-Debug-Trybot

Committed: https://skia.googlesource.com/skia/+/c5e0891029bed0f9619d67281a81f13983a9687b

[mtklein_C, 2015-04-09 21:39:39]

mtklein@chromium.org changed reviewers:
+ msarett@google.com, reed@google.com

[mtklein_C, 2015-04-09 21:41:33]

The CQ bit was checked by mtklein@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-09 21:41:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1075003002/1

[commit-bot: I haz the power, 2015-04-09 21:49:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-09 21:49:05]

This issue passed the CQ dry run.

[mtklein, 2015-04-09 22:35:45]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-04-09 22:35:45]

Incidentally, all testing was on N7.  As I was working on this, I finally
realized why the N9 was behaving so oddly earlier today... we're using the
portable code (autovectorized) on ARM64.  These old .S files are not compatible
with ARM64.  But the new intrinsics are!  When we get this particular CL sorted
out, I'll take a look at turning these procs on for ARM64.

[msarett, 2015-04-10 12:23:31]

lgtm with questions and comments.

Looking forward to hearing about ARM64.

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
File src/opts/SkUtils_opts_arm_neon.cpp (right):

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
src/opts/SkUtils_opts_arm_neon.cpp:13: uint32x4x4_t v16 = { v4, v4, v4, v4 };
Does this operation involve copying the contents of the register that contains
v4 to 3 other registers?

If so, do you think this is a lot of overhead when count is less than 16?

I'm guessing that when count is small memset is very fast regardless, so we are
probably optimizing for the case when count is very large?  In that case, this
seems like a good strategy.

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
src/opts/SkUtils_opts_arm_neon.cpp:16: vst4q_u32(dst, v16);  // This swizzles,
but we don't care: all lanes are the same, value.
I just read a little more about ARM deinterleaving loads and interleaving
stores, since I've never really used or seen these before.

It looks like they were designed specifically for processing RGBA data.  Might
be good to be on the lookout for places that we can take advantage of these
instructions, where we might actually want the interleaving.

Also, as far as I can tell and as you probably discovered as well, there doesn't
seem to be any multi-element loads/stores that don't interleave, which could be
frustrating in a situation where we didn't want to interleave.

Have you checked that this is faster than four calls to a normal 1 vector store?
 I would bet it is, but I just wanted to ask in case there is a reason ARM
doesn't provide a non-interleaving 4 element store.

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
src/opts/SkUtils_opts_arm_neon.cpp:28: vst1_u32(dst, vget_low_u32(v4));
vget_low_u32 is a free call, right?

[mtklein, 2015-04-10 12:45:56]

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
File src/opts/SkUtils_opts_arm_neon.cpp (right):

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
src/opts/SkUtils_opts_arm_neon.cpp:13: uint32x4x4_t v16 = { v4, v4, v4, v4 };
On 2015/04/10 12:23:31, msarett wrote:
> Does this operation involve copying the contents of the register that contains
> v4 to 3 other registers?
Yes.
 
> If so, do you think this is a lot of overhead when count is less than 16?
> 
> I'm guessing that when count is small memset is very fast regardless, so we
are
> probably optimizing for the case when count is very large?  In that case, this
> seems like a good strategy.

Right.  We can actually assert count >= 10 today.  Lower than that and we'll do
an inline loop in C rather than going through the overhead of CPU feature
detection and function pointers.  (Just added that yesterday.)

I did try guarding the 16-at-a-time section with an if (count >= 16), and it
seemed no better.

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
src/opts/SkUtils_opts_arm_neon.cpp:16: vst4q_u32(dst, v16);  // This swizzles,
but we don't care: all lanes are the same, value.
On 2015/04/10 12:23:31, msarett wrote:
> I just read a little more about ARM deinterleaving loads and interleaving
> stores, since I've never really used or seen these before.
> 
> It looks like they were designed specifically for processing RGBA data.  Might
> be good to be on the lookout for places that we can take advantage of these
> instructions, where we might actually want the interleaving.
Yep.  You'll find them used quite a bit under src/opts.  I'd love to find a way
to incorporate something like this into our SkPMFloat model without hurting too
much on Intel.

> 
> Also, as far as I can tell and as you probably discovered as well, there
doesn't
> seem to be any multi-element loads/stores that don't interleave, which could
be
> frustrating in a situation where we didn't want to interleave.
Yes, not in intrinsics.  In assembly, you can do them almost arbitrarily, all
the way up to something like vst1.u32 {d0,d1,d2,d3,d4,d5,d6,d7}, [r0] to do a
giant cache-line write to a pointer.

> 
> Have you checked that this is faster than four calls to a normal 1 vector
store?
>  I would bet it is, but I just wanted to ask in case there is a reason ARM
> doesn't provide a non-interleaving 4 element store.

Yep.  As far as I could tell, using vst4q is what pushed me down into the range
where we're competitive with the old assembly.  It is faster than the best code
I could get out of 4 vst1q_u32 calls.  It's not clear to me that's faster than
the best code generated possible using vst1.32 instructions (i.e. inline
assembly), but I very much want to stick to intrinsics for maintainability and
portability across ARM versions.

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
src/opts/SkUtils_opts_arm_neon.cpp:28: vst1_u32(dst, vget_low_u32(v4));
On 2015/04/10 12:23:31, msarett wrote:
> vget_low_u32 is a free call, right?

Yep.  It just translates from whatever q-register we're in to the corresponding
d-register.  (vget_high is free too for the same reason.)

[mtklein, 2015-04-10 12:51:44]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-04-10 12:51:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1075003002/1

[msarett, 2015-04-10 12:55:10]

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
File src/opts/SkUtils_opts_arm_neon.cpp (right):

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
src/opts/SkUtils_opts_arm_neon.cpp:13: uint32x4x4_t v16 = { v4, v4, v4, v4 };
On 2015/04/10 12:45:55, mtklein wrote:
> On 2015/04/10 12:23:31, msarett wrote:
> > Does this operation involve copying the contents of the register that
contains
> > v4 to 3 other registers?
> Yes.
>  
> > If so, do you think this is a lot of overhead when count is less than 16?
> > 
> > I'm guessing that when count is small memset is very fast regardless, so we
> are
> > probably optimizing for the case when count is very large?  In that case,
this
> > seems like a good strategy.
> 
> Right.  We can actually assert count >= 10 today.  Lower than that and we'll
do
> an inline loop in C rather than going through the overhead of CPU feature
> detection and function pointers.  (Just added that yesterday.)
> 
> I did try guarding the 16-at-a-time section with an if (count >= 16), and it
> seemed no better.

Cool good to know!

https://codereview.chromium.org/1075003002/diff/1/src/opts/SkUtils_opts_arm_n...
src/opts/SkUtils_opts_arm_neon.cpp:16: vst4q_u32(dst, v16);  // This swizzles,
but we don't care: all lanes are the same, value.
On 2015/04/10 12:45:55, mtklein wrote:
> On 2015/04/10 12:23:31, msarett wrote:
> > I just read a little more about ARM deinterleaving loads and interleaving
> > stores, since I've never really used or seen these before.
> > 
> > It looks like they were designed specifically for processing RGBA data. 
Might
> > be good to be on the lookout for places that we can take advantage of these
> > instructions, where we might actually want the interleaving.
> Yep.  You'll find them used quite a bit under src/opts.  I'd love to find a
way
> to incorporate something like this into our SkPMFloat model without hurting
too
> much on Intel.
> 
> > 
> > Also, as far as I can tell and as you probably discovered as well, there
> doesn't
> > seem to be any multi-element loads/stores that don't interleave, which could
> be
> > frustrating in a situation where we didn't want to interleave.
> Yes, not in intrinsics.  In assembly, you can do them almost arbitrarily, all
> the way up to something like vst1.u32 {d0,d1,d2,d3,d4,d5,d6,d7}, [r0] to do a
> giant cache-line write to a pointer.
> 
> > 
> > Have you checked that this is faster than four calls to a normal 1 vector
> store?
> >  I would bet it is, but I just wanted to ask in case there is a reason ARM
> > doesn't provide a non-interleaving 4 element store.
> 
> Yep.  As far as I could tell, using vst4q is what pushed me down into the
range
> where we're competitive with the old assembly.  It is faster than the best
code
> I could get out of 4 vst1q_u32 calls.  It's not clear to me that's faster than
> the best code generated possible using vst1.32 instructions (i.e. inline
> assembly), but I very much want to stick to intrinsics for maintainability and
> portability across ARM versions.

That makes sense.  +1 for sticking with intrinsics.  The readability of this
code vs the assembly is a tremendous gain.

[commit-bot: I haz the power, 2015-04-10 13:25:01]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/c5e0891029bed0f9619d67281a81f13983a9687b