ARM Skia NEON patches - 31 - Xfermode: xfer16

ARM Skia NEON patches - 31 - Xfermode: xfer16
Xfermode: xfer16

This adds support for 16bit Xfermodes. It also tunes the gcc test
macros in xfer32() to add compatibility for gcc > 4.

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

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=12192

[kevin.petit.not.used.account, 2013-11-06 15:09:04]

ping.

This should be fairly uncontroversial:
- no mismatches
- big speedup on 16bit xfermodes
- contains the fix for gcc 5.x you asked for

[reed1, 2013-11-06 15:35:38]

derek/mike, what try-bots should we run?

https://codereview.chromium.org/33063002/diff/1/src/opts/SkColor_opts_neon.h
File src/opts/SkColor_opts_neon.h (right):

https://codereview.chromium.org/33063002/diff/1/src/opts/SkColor_opts_neon.h#...
src/opts/SkColor_opts_neon.h:32: static inline uint8x8x4_t
SkPixel16ToPixel32_neon8(uint16x8_t vsrc) {
ca we add a brief comment above each of these?

e.g.

// input: 8 RGB565 pixels stored (in what packing order?) in vsrc
// output 8 SkPMColor pixels, expanded by bit-replication? (in what packing
order?)

[mtklein, 2013-11-06 15:38:46]

On 2013/11/06 15:35:38, reed1 wrote:
> derek/mike, what try-bots should we run?
> 
> https://codereview.chromium.org/33063002/diff/1/src/opts/SkColor_opts_neon.h
> File src/opts/SkColor_opts_neon.h (right):
> 
>
https://codereview.chromium.org/33063002/diff/1/src/opts/SkColor_opts_neon.h#...
> src/opts/SkColor_opts_neon.h:32: static inline uint8x8x4_t
> SkPixel16ToPixel32_neon8(uint16x8_t vsrc) {
> ca we add a brief comment above each of these?
> 
> e.g.
> 
> // input: 8 RGB565 pixels stored (in what packing order?) in vsrc
> // output 8 SkPMColor pixels, expanded by bit-replication? (in what packing
> order?)

Nexus 4 is almost always a good choice.

[mtklein, 2013-11-06 15:54:41]

On 2013/11/06 15:38:46, mtklein wrote:
> On 2013/11/06 15:35:38, reed1 wrote:
> > derek/mike, what try-bots should we run?
> > 
> > https://codereview.chromium.org/33063002/diff/1/src/opts/SkColor_opts_neon.h
> > File src/opts/SkColor_opts_neon.h (right):
> > 
> >
>
https://codereview.chromium.org/33063002/diff/1/src/opts/SkColor_opts_neon.h#...
> > src/opts/SkColor_opts_neon.h:32: static inline uint8x8x4_t
> > SkPixel16ToPixel32_neon8(uint16x8_t vsrc) {
> > ca we add a brief comment above each of these?
> > 
> > e.g.
> > 
> > // input: 8 RGB565 pixels stored (in what packing order?) in vsrc
> > // output 8 SkPMColor pixels, expanded by bit-replication? (in what packing
> > order?)
> 
> Nexus 4 is almost always a good choice.

This lg2m.  Just an extra request or two on top of Mike's.

Given that there are no mismatches, and this is a fairly obscure code path, are
you sure we're actually testing it?  Do you know which GMs trigger this code
path?

[mtklein, 2013-11-06 16:04:16]

Whoops, forgot to send them...

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

https://codereview.chromium.org/33063002/diff/1/src/opts/SkXfermode_opts_arm_...
src/opts/SkXfermode_opts_arm_neon.cpp:648: SkXfermodeProcSIMD procSIMD =
reinterpret_cast<SkXfermodeProcSIMD>(fProcSIMD);
SkASSERT procSIMD != NULL?

https://codereview.chromium.org/33063002/diff/1/src/opts/SkXfermode_opts_arm_...
src/opts/SkXfermode_opts_arm_neon.cpp:657: #if (__GNUC__ > 4) || ((__GNUC__ ==
4) && (__GNUC_MINOR__ > 6))
Is Clang any better at generating code from intrinsics?  I'm now officially in
your camp that GCC just barfs out garbage.  (Even up to 4.8 as far as I've
seen).

[kevin.petit.not.used.account, 2013-11-06 16:46:02]

On 2013/11/06 15:54:41, mtklein wrote:
> On 2013/11/06 15:38:46, mtklein wrote:
> > On 2013/11/06 15:35:38, reed1 wrote:
> > > derek/mike, what try-bots should we run?
> > Nexus 4 is almost always a good choice.
> 
> This lg2m.  Just an extra request or two on top of Mike's.
> 
> Given that there are no mismatches, and this is a fairly obscure code path,
are
> you sure we're actually testing it?  Do you know which GMs trigger this code
> path?

Yes, at least the xfermode GMs trigger this code path (--config 565). bench also
triggers this code path.

[kevin.petit.not.used.account, 2013-11-06 16:50:05]

https://codereview.chromium.org/33063002/diff/1/src/opts/SkColor_opts_neon.h
File src/opts/SkColor_opts_neon.h (right):

https://codereview.chromium.org/33063002/diff/1/src/opts/SkColor_opts_neon.h#...
src/opts/SkColor_opts_neon.h:32: static inline uint8x8x4_t
SkPixel16ToPixel32_neon8(uint16x8_t vsrc) {
If you want. Before I submit another patch I'd like to be sure I precisely
understand what you want.

SkPMColor: thanks to the NEON_* defines it will handle transparently all
possible configurations (little/big endian and ARGB/ABGR for example).

RGB565: I haven't seen anything (in Skia) else than (from high to low) R, G, B.
The code here, even if it uses all the SK_?16_BITS defines, relies on that
assumption.

My proposal is to add something like:

"This function expands 8 pixels from RGB565 (R, G, B from high to low) to
SkPMColor (all possible configurations supported) in the exact same way as
SkPixel16ToPixel32."

And:

"This function packs 8 pixels from SkPMColor (all possible configurations
supported) to RGB565 (R, G, B from high to low) in the exact same way as
SkPixel32ToPixel16."

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

https://codereview.chromium.org/33063002/diff/1/src/opts/SkXfermode_opts_arm_...
src/opts/SkXfermode_opts_arm_neon.cpp:648: SkXfermodeProcSIMD procSIMD =
reinterpret_cast<SkXfermodeProcSIMD>(fProcSIMD);
Why not but it isn't possible to reach this code with a NULL fProcSIMD. You can
have a look at SkPlatformXfermodeFactory_impl_neon() to convince yourself.

If the intent is to document that and or protect ourselves from future
modifications, then yes (plus modifying the above function I guess).

https://codereview.chromium.org/33063002/diff/1/src/opts/SkXfermode_opts_arm_...
src/opts/SkXfermode_opts_arm_neon.cpp:657: #if (__GNUC__ > 4) || ((__GNUC__ ==
4) && (__GNUC_MINOR__ > 6))
To be honest I haven't done a detailed analysis of the code generated by Clang.

I know that gcc, starting from 4.7 generates pretty good code for most of the
intrinsics as long as the pressure on registers is reasonable (read as long as
the function isn't too long and doesn't use simultaneously too many NEON
variables). If that's not the case it starts doing lots of stupid things.

However vld4 is a remarkable counter-example that even 4.8 doesn't always handle
in an optimal way.

If you have examples of bad code generated by gcc or clang, I'd be more than
interested.

[reed1, 2013-11-06 16:51:44]

that sort of text for a comment looks good, thanks

[mtklein, 2013-11-06 17:18:40]

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

https://codereview.chromium.org/33063002/diff/1/src/opts/SkXfermode_opts_arm_...
src/opts/SkXfermode_opts_arm_neon.cpp:648: SkXfermodeProcSIMD procSIMD =
reinterpret_cast<SkXfermodeProcSIMD>(fProcSIMD);
On 2013/11/06 16:50:05, kevin.petit.arm wrote:
> Why not but it isn't possible to reach this code with a NULL fProcSIMD. You
can
> have a look at SkPlatformXfermodeFactory_impl_neon() to convince yourself.
> 
> If the intent is to document that and or protect ourselves from future
> modifications, then yes (plus modifying the above function I guess).

Yeah, the intention is just documentation.

https://codereview.chromium.org/33063002/diff/1/src/opts/SkXfermode_opts_arm_...
src/opts/SkXfermode_opts_arm_neon.cpp:657: #if (__GNUC__ > 4) || ((__GNUC__ ==
4) && (__GNUC_MINOR__ > 6))
On 2013/11/06 16:50:05, kevin.petit.arm wrote:
> To be honest I haven't done a detailed analysis of the code generated by
Clang.
> 
> I know that gcc, starting from 4.7 generates pretty good code for most of the
> intrinsics as long as the pressure on registers is reasonable (read as long as
> the function isn't too long and doesn't use simultaneously too many NEON
> variables). If that's not the case it starts doing lots of stupid things.
> 
> However vld4 is a remarkable counter-example that even 4.8 doesn't always
handle
> in an optimal way.
> 
> If you have examples of bad code generated by gcc or clang, I'd be more than
> interested.

Ah, this may be a coincidence then.  I was indeed thinking of the bizarre code
GCC generates for loads.  I see things like, load into d16-d17, followed
immediately by storing d16 and d17 back to the stack.

[kevin.petit.not.used.account, 2013-11-07 11:26:20]

Patch Set 2 addresses comments received so far.

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

https://codereview.chromium.org/33063002/diff/1/src/opts/SkXfermode_opts_arm_...
src/opts/SkXfermode_opts_arm_neon.cpp:648: SkXfermodeProcSIMD procSIMD =
reinterpret_cast<SkXfermodeProcSIMD>(fProcSIMD);
On 2013/11/06 17:18:41, mtklein wrote:
> On 2013/11/06 16:50:05, kevin.petit.arm wrote:
> > Why not but it isn't possible to reach this code with a NULL fProcSIMD. You
> can
> > have a look at SkPlatformXfermodeFactory_impl_neon() to convince yourself.
> > 
> > If the intent is to document that and or protect ourselves from future
> > modifications, then yes (plus modifying the above function I guess).
> 
> Yeah, the intention is just documentation.

Done.

https://codereview.chromium.org/33063002/diff/1/src/opts/SkXfermode_opts_arm_...
src/opts/SkXfermode_opts_arm_neon.cpp:657: #if (__GNUC__ > 4) || ((__GNUC__ ==
4) && (__GNUC_MINOR__ > 6))
On 2013/11/06 17:18:41, mtklein wrote:
> Ah, this may be a coincidence then.  I was indeed thinking of the bizarre code
> GCC generates for loads.  I see things like, load into d16-d17, followed
> immediately by storing d16 and d17 back to the stack.

Yes, that's a typical pattern. Could you point me to the code producing this
assembly please?

As a general remark, don't hesitate to drop me a line with code snippets when
you stumble upon bad assembly produced by gcc or clang. Our compiler team and
myself are very keen on getting that kind of feedback.

[kevin.petit.not.used.account, 2013-11-08 15:44:36]

Any more comments on this one?

[mtklein, 2013-11-08 15:56:28]

On 2013/11/08 15:44:36, kevin.petit.arm wrote:
> Any more comments on this one?

Sorry for the delay (and thanks for pinging).  lgtm

[commit-bot: I haz the power, 2013-11-08 15:57:39]

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

[commit-bot: I haz the power, 2013-11-08 16:16:08]

Change committed as 12192