Cleanup with SkAlphaMulQ_SSE2()

Cleanup with SkAlphaMulQ_SSE2()

Related nanobench results:
before:
  10M   18      7.03µs  7.31µs  7.38µs  8.46µs  6%      ▂▁▂▂▂▃▄▁█▁      8888    bitmaprect_80_filter_identity
  10M   43      6.96µs  6.97µs  6.99µs  7.19µs  1%      ▁▂▁▁▁▁▁█▁▁      8888    bitmaprect_80_nofilter_identity
  10M   14      35.7µs  35.8µs  35.9µs  36.3µs  1%      ▃▂▁▂▁█▂▁▁▁      8888    bitmap_BGRA_8888_update_scale_bilerp
  10M   16      35.5µs  35.6µs  35.7µs  36.3µs  1%      █▅▂▁▁▁▃▂▁▁      8888    bitmap_BGRA_8888_update_volatile_scale_bilerp
  10M   16      35.4µs  35.4µs  35.5µs  36.8µs  1%      ▂▁█▁▁▁▁▂▁▁      8888    bitmap_BGRA_8888_scale_bilerp
  10M   25      16.4µs  16.6µs  16.7µs  17.4µs  2%      ▂▁▁▂▁▁▁▅▅█      8888    bitmap_Index_8
  10M   15      37.9µs  38µs    38µs    38.4µs  0%      ▄▆▂▁▁▁█▂▁▁      8888    bitmap_RGB_565
  10M   33      11.1µs  11.1µs  11.1µs  11.2µs  0%      ▆▂█▂▂▂▁▁▂▁      8888    bitmap_BGRA_8888_scale
after:
  10M   9       7.04µs  7.06µs  7.1µs   7.32µs  1%      █▅▂▁▁▂▁▁▁▁      8888    bitmaprect_80_filter_identity
  10M   18      7.01µs  7.02µs  7.05µs  7.25µs  1%      █▂▁▁▁▁▁▁▁▁      8888    bitmaprect_80_nofilter_identity
  10M   5       33.9µs  34µs    34.1µs  34.5µs  1%      █▃▂▂▁▁▁▅▃▂      8888    bitmap_BGRA_8888_update_scale_bilerp
  10M   7       35.5µs  35.5µs  35.6µs  36.3µs  1%      ▃▂▂▁▂▁▂▁█▂      8888    bitmap_BGRA_8888_update_volatile_scale_bilerp
  10M   7       35.5µs  35.5µs  35.7µs  36.8µs  1%      ▂▁▁▁▁▁▁▁▁█      8888    bitmap_BGRA_8888_scale_bilerp
  10M   11      16.4µs  16.4µs  16.4µs  16.6µs  0%      █▂▁▁▂▁▁▁▂▁      8888    bitmap_Index_8
  10M   7       37.3µs  37.4µs  38.4µs  47.8µs  9%      ▁▁▁▁▁▁▁▁▁█      8888    bitmap_RGB_565
  10M   33      11µs    11µs    11.1µs  11.2µs  1%      ▄█▅▃▂▁▁▁▁▁      8888    bitmap_BGRA_8888_scale


BUG=skia:

Committed: https://skia.googlesource.com/skia/+/533a32782f9817bb307484b36323040470575da4

[qiankun, 2014-11-24 06:50:43]

qiankun.miao@intel.com changed reviewers:
+ mtklein@google.com

[qiankun, 2014-11-24 06:50:43]

PTAL

[reed1, 2014-11-24 14:28:54]

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

[reed1, 2014-11-24 14:28:54]

can you add nanobench results for the relevant tests before/after this CL?

[qiankun, 2014-11-24 15:32:28]

Thanks for review. I updated the nanobench data.

[reed1, 2014-11-24 16:18:52]

bitmaprect_80_nofilter_identity is probably the key bench to track. It uses the
function in question (S32_Blend_BlitRow32_SSE2 95%.

[mtklein, 2014-11-24 16:48:23]

https://codereview.chromium.org/755573002/diff/1/src/opts/SkBlitRow_opts_SSE2...
File src/opts/SkBlitRow_opts_SSE2.cpp (left):

https://codereview.chromium.org/755573002/diff/1/src/opts/SkBlitRow_opts_SSE2...
src/opts/SkBlitRow_opts_SSE2.cpp:70: __m128i src_ag = _mm_and_si128(ag_mask,
src_pixel);
So, it seems like the changes are:
  1) for AG, rshift, mul, lshift instead of mask, mul, mask
  2) do AG/RB or'ing eagerly instead of lazily just before the add.

Is 1) to avoid the need for a separate AG mask, or something more subtle? 
Couldn't we use _mm_andnot_si128 with the RB mask?  Is the note about Atom
execution ports still relevant?  Were we perhaps using masks instead of shifts
to avoid contending on the multiply port?

Do you think 2) makes any difference?  Seems like no?  Couldn't the compiler
still delay the ors if it thought they mattered?

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

https://codereview.chromium.org/755573002/diff/1/src/opts/SkColor_opts_SSE2.h...
src/opts/SkColor_opts_SSE2.h:66: // Fast path for colors scale the same factor.
Maybe  // Fast path for SkAlphaMulQ_SSE2 with a constant scale factor.

[qiankun, 2014-11-25 05:33:52]

Hi reviewers, thanks very much for your comments and advise. The performance
regression is fixed. Please see the data.

https://codereview.chromium.org/755573002/diff/1/src/opts/SkBlitRow_opts_SSE2...
File src/opts/SkBlitRow_opts_SSE2.cpp (left):

https://codereview.chromium.org/755573002/diff/1/src/opts/SkBlitRow_opts_SSE2...
src/opts/SkBlitRow_opts_SSE2.cpp:70: __m128i src_ag = _mm_and_si128(ag_mask,
src_pixel);
On 2014/11/24 16:48:23, mtklein wrote:
> So, it seems like the changes are:
>   1) for AG, rshift, mul, lshift instead of mask, mul, mask
>   2) do AG/RB or'ing eagerly instead of lazily just before the add.
> 
> Is 1) to avoid the need for a separate AG mask, or something more subtle? 
> Couldn't we use _mm_andnot_si128 with the RB mask?  Is the note about Atom
> execution ports still relevant?  Were we perhaps using masks instead of shifts
> to avoid contending on the multiply port?
> 
> Do you think 2) makes any difference?  Seems like no?  Couldn't the compiler
> still delay the ors if it thought they mattered?

Get the performance back using mask, mul, mask instead of rshift, mul, lshift. I
used the later intrinsics at first because without moving rb_mask/ag_mask
assignment out of SkAlphaMulQ_SSE2() performance of the previous intrinsics are
worse.

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

https://codereview.chromium.org/755573002/diff/1/src/opts/SkColor_opts_SSE2.h...
src/opts/SkColor_opts_SSE2.h:66: // Fast path for colors scale the same factor.
On 2014/11/24 16:48:23, mtklein wrote:
> Maybe  // Fast path for SkAlphaMulQ_SSE2 with a constant scale factor.

Done.

[qiankun, 2014-11-25 08:11:07]

Patchset #3 (id:40001) has been deleted

[mtklein, 2014-11-25 13:48:31]

The CQ bit was checked by mtklein@google.com

[mtklein, 2014-11-25 13:48:31]

lgtm

[commit-bot: I haz the power, 2014-11-25 13:48:58]

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

[commit-bot: I haz the power, 2014-11-25 13:59:42]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/533a32782f9817bb307484b36323040470575da4

[Tom Hudson, 2014-11-25 14:21:49]

In practice, this seems to have improved the _scale_ tests a little, but reed@
expected it to be helping bitmaprect_80_nofilter_identity?
Particularly when there isn't a bug to refer to, I'd like to see what you
expected to improve in future commit messages, as well as the raw data.

[qiankun, 2014-11-25 14:47:36]

On 2014/11/25 14:21:49, Tom Hudson wrote:
> In practice, this seems to have improved the _scale_ tests a little, but reed@
> expected it to be helping bitmaprect_80_nofilter_identity?
> Particularly when there isn't a bug to refer to, I'd like to see what you
> expected to improve in future commit messages, as well as the raw data.

I wrapped some common operations into functions in this CL for easy maintenance.
Expected there was not regression. It would be better to improve some
benchmarks. I will continue to keep an eye on performance improvements of these
tests.

[mtklein, 2014-11-25 15:00:11]

It's important to remember that we do not care what the bots say about
microbenchmarks.  They change on whim.  Microbenchmarks exist for exactly how we
just used them, to double check what we're doing makes sense in the context of a
single change, usually on a single machine.  If skiaperf.com detects an .skp
regresses or improves, then that's something to worry about.

Qiankun, your methodology here was absolutely ideal.  If everyone else sent me
CLs like this, I'd be a much happier reviewer.

[reed1, 2014-11-25 15:01:09]

On 2014/11/25 14:47:36, qiankun wrote:
> On 2014/11/25 14:21:49, Tom Hudson wrote:
> > In practice, this seems to have improved the _scale_ tests a little, but
reed@
> > expected it to be helping bitmaprect_80_nofilter_identity?
> > Particularly when there isn't a bug to refer to, I'd like to see what you
> > expected to improve in future commit messages, as well as the raw data.
> 
> I wrapped some common operations into functions in this CL for easy
maintenance.
> Expected there was not regression. It would be better to improve some
> benchmarks. I will continue to keep an eye on performance improvements of
these
> tests.

I agree that the CL simplifies the code, which is good. The timings that were
posted show a slight slowdown, but I presume we think that was just noise?

[mtklein, 2014-11-25 15:04:29]

> I agree that the CL simplifies the code, which is good. The timings that were
> posted show a slight slowdown, but I presume we think that was just noise?

Yes, the latest updated times in the description are neutral.