Replace SSE optimization of Color32A_D565

Replace SSE optimization of Color32A_D565

Adds an SSE2 version of the Color32A_D565 function, to replace
the existing SSE4 version. Also does some minor cleanup.

Performance improvement in the following Skia benchmarks.
Measured on Atom Silvermont:
  Xfermode_SrcOver       - x3
  luma_colorfilter_large - x4.6
  luma_colorfilter_small - x2
  tablebench             - ~15%
  chart_bw               - ~10%

Measured on Corei7 Haswell:
luma_colorfilter_large running SSE2 - x2
luma_colorfilter_large running SSE4 - x2.3

Also improves performance in WPS Office application and 2D subtest of 0xbenchmark on Android.

Signed-off-by: Henrik Smiding <henrik.smiding@intel.com>;

Committed: https://skia.googlesource.com/skia/+/70840cbd898df67f603987213164c798415d76bf

[henrik.smiding, 2015-02-12 13:19:38]

henrik.smiding@intel.com changed reviewers:
+ joakim.landberg@intel.com, mtklein@google.com, reed@google.com

[henrik.smiding, 2015-02-12 13:27:10]

Surprisingly, the SSE2 version proved to be much faster on the Atom Silvermont
CPU, compared to SSE4. The 32-bit MULLD multiplication instruction used in the
SSE4 version is 11 cycles on Silvermont, compared to 1-2 on Core i7.

I have moved the common code to ColorPriv.h, and cleaned up som header files as
well.

[reed1, 2015-02-12 14:44:06]

https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:289: static inline U16CPU SkBlend32A_D565(uint16_t
dst, unsigned scale, uint32_t src_expand) {
Why is there 32A in the name? Can you pick a name that better reflects the
parameter types? Also, Do functions in this file usually place src first or dst
first?

[mtklein, 2015-02-12 14:56:31]

https://codereview.chromium.org/923523002/diff/1/src/opts/opts_check_x86.cpp
File src/opts/opts_check_x86.cpp (right):

https://codereview.chromium.org/923523002/diff/1/src/opts/opts_check_x86.cpp#...
src/opts/opts_check_x86.cpp:234: #if !defined(__slm__)
Is this __slm__ for SiLverMont?  Is there a way to detect we're compiling for
Atom or Core more generally?  I know Silvermont is the only Atom chip with SSE4
today, but if we're going to start down this path, it might make sense to
generalize this guard up to the whole family.

Another option is to simply remove the SSE4 path for now.  It doesn't seem like
SSE4 makes a huge difference on Haswell (an extra 15% improvement), and we're
not really expecting much coverage of 565 on non-mobile.

[henrik.smiding, 2015-02-12 15:45:06]

https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:289: static inline U16CPU SkBlend32A_D565(uint16_t
dst, unsigned scale, uint32_t src_expand) {
On 2015/02/12 14:44:06, reed1 wrote:
> Why is there 32A in the name? Can you pick a name that better reflects the
> parameter types? Also, Do functions in this file usually place src first or
dst
> first?

It was hard to come up with a good name. The method blends an expanded rgb888
color value with a scaled rgb565, and returns rgb565. Would SkBlend32_RGB16 make
more sense? Or SkBlendRGB32_RGB16? But the source isn't in a standard ARGB8888
format either...

Should I move this to BlitRow.h instead? Since it works with a pre-expanded
24-bit color value, to match the SkExpand_rgb_16/SkCompact_rgb_16 above. Not a
common format.

I will change the order of the parameters.

https://codereview.chromium.org/923523002/diff/1/src/opts/opts_check_x86.cpp
File src/opts/opts_check_x86.cpp (right):

https://codereview.chromium.org/923523002/diff/1/src/opts/opts_check_x86.cpp#...
src/opts/opts_check_x86.cpp:234: #if !defined(__slm__)
On 2015/02/12 14:56:31, mtklein wrote:
> Is this __slm__ for SiLverMont?  Is there a way to detect we're compiling for
> Atom or Core more generally?  I know Silvermont is the only Atom chip with
SSE4
> today, but if we're going to start down this path, it might make sense to
> generalize this guard up to the whole family.
> 
> Another option is to simply remove the SSE4 path for now.  It doesn't seem
like
> SSE4 makes a huge difference on Haswell (an extra 15% improvement), and we're
> not really expecting much coverage of 565 on non-mobile.

Yes, it's one of the defines set by gcc/llvm when targeting Silvermont
architecture. I haven't found any better flag to check. __atom__ is only set
when targeting pre-Silvermonts. I also don't know when or if gcc/llvm will
support future *mont archs targets. It felt overkill to check this with cpuid.
It might make sense to change it to check the ANDROID flag instead?

Do you mean I should remove the entire SSE4 function, or just the call in
opts_check_x86.cpp?
It would remove the need for the Silvermont 'special exception', and the
performance of the SSE2 version is still very good.

[mtklein, 2015-02-12 15:49:57]

On 2015/02/12 15:45:06, henrik.smiding wrote:
> https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h
> File include/core/SkColorPriv.h (right):
> 
>
https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h#n...
> include/core/SkColorPriv.h:289: static inline U16CPU SkBlend32A_D565(uint16_t
> dst, unsigned scale, uint32_t src_expand) {
> On 2015/02/12 14:44:06, reed1 wrote:
> > Why is there 32A in the name? Can you pick a name that better reflects the
> > parameter types? Also, Do functions in this file usually place src first or
> dst
> > first?
> 
> It was hard to come up with a good name. The method blends an expanded rgb888
> color value with a scaled rgb565, and returns rgb565. Would SkBlend32_RGB16
make
> more sense? Or SkBlendRGB32_RGB16? But the source isn't in a standard ARGB8888
> format either...
> 
> Should I move this to BlitRow.h instead? Since it works with a pre-expanded
> 24-bit color value, to match the SkExpand_rgb_16/SkCompact_rgb_16 above. Not a
> common format.
> 
> I will change the order of the parameters.
> 
> https://codereview.chromium.org/923523002/diff/1/src/opts/opts_check_x86.cpp
> File src/opts/opts_check_x86.cpp (right):
> 
>
https://codereview.chromium.org/923523002/diff/1/src/opts/opts_check_x86.cpp#...
> src/opts/opts_check_x86.cpp:234: #if !defined(__slm__)
> On 2015/02/12 14:56:31, mtklein wrote:
> > Is this __slm__ for SiLverMont?  Is there a way to detect we're compiling
for
> > Atom or Core more generally?  I know Silvermont is the only Atom chip with
> SSE4
> > today, but if we're going to start down this path, it might make sense to
> > generalize this guard up to the whole family.
> > 
> > Another option is to simply remove the SSE4 path for now.  It doesn't seem
> like
> > SSE4 makes a huge difference on Haswell (an extra 15% improvement), and
we're
> > not really expecting much coverage of 565 on non-mobile.
> 
> Yes, it's one of the defines set by gcc/llvm when targeting Silvermont
> architecture. I haven't found any better flag to check. __atom__ is only set
> when targeting pre-Silvermonts. I also don't know when or if gcc/llvm will
> support future *mont archs targets. It felt overkill to check this with cpuid.
> It might make sense to change it to check the ANDROID flag instead?
Gotcha.

> Do you mean I should remove the entire SSE4 function, or just the call in
> opts_check_x86.cpp?
> It would remove the need for the Silvermont 'special exception', and the
> performance of the SSE2 version is still very good.
Right, I'm suggesting we remove the entire SSE4 path, and leave a note here
in opts_check_x86 that says, "If you're thinking about writing an SSE4 version
of this, do check it's actually faster on Atom.  Our original SSE4 version was
slower than this SSE2 version on Silvermont, and only marginally faster on a
Core i7."

[henrik.smiding, 2015-02-13 14:48:09]

I've removed the SSE4 code, in favor of the SSE2. Even if it's a 0-30% drop in
performance on a Core CPU, they probably don't use 16-bit color maps that much,
like you said. *sniffle*

[mtklein, 2015-02-13 15:15:04]

This lgtm.  Let's make sure Mike's happy with SkColorPriv.h (and that the rest
is consistently named if that changes) before submitting.

There's no better contribution to Skia than deleting code that doesn't need to
be there, and it doesn't diminish the contribution of writing that code in the
first place.

I just noticed how much clearer the SSE2 code is now too.  Seems like a nice
side effect of being stuck using _mm_mullo_epi16.  :)

https://codereview.chromium.org/923523002/diff/20001/src/core/SkBlitRow_D16.cpp
File src/core/SkBlitRow_D16.cpp (right):

https://codereview.chromium.org/923523002/diff/20001/src/core/SkBlitRow_D16.c...
src/core/SkBlitRow_D16.cpp:271: // no need for the addition code specializing on
opaque alpha at this time
addition -> additional?

[henrik.smiding, 2015-02-13 15:33:29]

On 2015/02/13 15:15:04, mtklein wrote:
> This lgtm.  Let's make sure Mike's happy with SkColorPriv.h (and that the rest
> is consistently named if that changes) before submitting.
> 
> There's no better contribution to Skia than deleting code that doesn't need to
> be there, and it doesn't diminish the contribution of writing that code in the
> first place.
> 
> I just noticed how much clearer the SSE2 code is now too.  Seems like a nice
> side effect of being stuck using _mm_mullo_epi16.  :)
> 
>
https://codereview.chromium.org/923523002/diff/20001/src/core/SkBlitRow_D16.cpp
> File src/core/SkBlitRow_D16.cpp (right):
> 
>
https://codereview.chromium.org/923523002/diff/20001/src/core/SkBlitRow_D16.c...
> src/core/SkBlitRow_D16.cpp:271: // no need for the addition code specializing
on
> opaque alpha at this time
> addition -> additional?

Yes, the SSE2 code is much 'simpler' and easier to understand. It took me less
than a day to write, compared to a week for the SSE4 version. That's why I'm
whining :)

On that subject, I plan to clean up the ColorRect32_SSE2 function. It's two
entire files that are unused right now. I don't know why it was disabled in the
first place, but I remember disabling the entire check in opts_check_x86 some
time ago. Since the code was not running the optimized path anyway. The comment
in the function is "has been disabled since this was added in r3423". Not sure
if it's even needed anymore. The fallback might be just as fast, or just using
memset32.

[henrik.smiding, 2015-02-13 15:38:11]

https://codereview.chromium.org/923523002/diff/20001/src/core/SkBlitRow_D16.cpp
File src/core/SkBlitRow_D16.cpp (right):

https://codereview.chromium.org/923523002/diff/20001/src/core/SkBlitRow_D16.c...
src/core/SkBlitRow_D16.cpp:271: // no need for the addition code specializing on
opaque alpha at this time
On 2015/02/13 15:15:04, mtklein wrote:
> addition -> additional?

Probably. Copy/paste from Mike's suggestion in 901593002.
Perhaps the comment could be changed to be: no need for the addition code
specializing on opaque alpha at this time...

[mtklein, 2015-02-13 15:41:15]

> On that subject, I plan to clean up the ColorRect32_SSE2 function. It's two
> entire files that are unused right now. I don't know why it was disabled in
the
> first place, but I remember disabling the entire check in opts_check_x86 some
> time ago. Since the code was not running the optimized path anyway. The
comment
> in the function is "has been disabled since this was added in r3423". Not sure
> if it's even needed anymore. The fallback might be just as fast, or just using
> memset32.

+1.  Until you mentioned it, I'd never really looked at that, and now that I'm
reading it, it's not clear to me we can beat just looping over rows.

[henrik.smiding, 2015-02-19 10:24:34]

On 2015/02/12 14:44:06, reed1 wrote:
> https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h
> File include/core/SkColorPriv.h (right):
> 
>
https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h#n...
> include/core/SkColorPriv.h:289: static inline U16CPU SkBlend32A_D565(uint16_t
> dst, unsigned scale, uint32_t src_expand) {
> Why is there 32A in the name? Can you pick a name that better reflects the
> parameter types? Also, Do functions in this file usually place src first or
dst
> first?

I have modified the name and parameter order to better match the rest of the
file.
Reed, please take a look.

[henrik.smiding, 2015-02-20 13:39:47]

New patchsets have been uploaded after l-g-t-m from mtklein@google.com

[henrik.smiding, 2015-02-26 11:40:58]

On 2015/02/19 10:24:34, henrik.smiding wrote:
> On 2015/02/12 14:44:06, reed1 wrote:
> > https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h
> > File include/core/SkColorPriv.h (right):
> > 
> >
>
https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h#n...
> > include/core/SkColorPriv.h:289: static inline U16CPU
SkBlend32A_D565(uint16_t
> > dst, unsigned scale, uint32_t src_expand) {
> > Why is there 32A in the name? Can you pick a name that better reflects the
> > parameter types? Also, Do functions in this file usually place src first or
> dst
> > first?
> 
> I have modified the name and parameter order to better match the rest of the
> file.
> Reed, please take a look.

Ping

[henrik.smiding, 2015-03-20 13:08:35]

On 2015/02/26 11:40:58, henrik.smiding wrote:
> On 2015/02/19 10:24:34, henrik.smiding wrote:
> > On 2015/02/12 14:44:06, reed1 wrote:
> > >
https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h
> > > File include/core/SkColorPriv.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/923523002/diff/1/include/core/SkColorPriv.h#n...
> > > include/core/SkColorPriv.h:289: static inline U16CPU
> SkBlend32A_D565(uint16_t
> > > dst, unsigned scale, uint32_t src_expand) {
> > > Why is there 32A in the name? Can you pick a name that better reflects the
> > > parameter types? Also, Do functions in this file usually place src first
or
> > dst
> > > first?
> > 
> > I have modified the name and parameter order to better match the rest of the
> > file.
> > Reed, please take a look.
> 
> Ping

What's the hold-up?

[reed1, 2015-03-20 13:16:39]

sorry, fell off my review-plate.
lgtm

[henrik.smiding, 2015-03-20 16:09:41]

The CQ bit was checked by henrik.smiding@intel.com

[commit-bot: I haz the power, 2015-03-20 16:09:56]

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

[commit-bot: I haz the power, 2015-03-20 16:20:51]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/70840cbd898df67f603987213164c798415d76bf