Optimize SkAlphaMulQ_SSE2

Optimize SkAlphaMulQ_SSE2

These two mask clear are useless, because _mm_srli_epi16 fills high byte
of each word with 0.

BUG=skia:

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

[qiankun, 2014-11-14 11:11:31]

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

[qiankun, 2014-11-14 11:11:32]

PTAL.

[mtklein, 2014-11-14 14:22:58]

Good find!

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

https://codereview.chromium.org/724333003/diff/1/src/opts/SkColor_opts_SSE2.h...
src/opts/SkColor_opts_SSE2.h:59: rb = _mm_and_si128(mask, rb);
Hmm.  It'd be nice to explain / assert this for readability.  Perhaps,
  static inline void assert_eq(__m128i a, __m128i b) {
    // Compare each byte pair for equality, then check all comparison bits are
true.
    SkASSERT(0xffff == _mm_movemask_epi8(_mm_cmpeq_epi8(a, b)));
  }

Then assert these masks are unnecessary:
  assert_eq(ag, _mm_and_sli128(mask, ag));  // _mm_srli_epi16(c,8) just did this
for us.
  ...
  assert_eq(rb, _mm_and_sli128(mask, rb));   // rb = _mm_srli_epi16(rb, 8) did
this for us.

I'm assuming that the arguments to assert_eq will not even be evaluated in
Release mode where SkASSERT() is a no-op.  If that's not the case, we could of
course write assert_eq as a macro:
  #define ASSERT_EQ(a,b) SkASSERT(0xffff ==
_mm_movemask_epi8(_mm_cmpeq_epi8((a), (b))))

[qiankun, 2014-11-14 15:35:23]

Thanks for advice.

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

https://codereview.chromium.org/724333003/diff/1/src/opts/SkColor_opts_SSE2.h...
src/opts/SkColor_opts_SSE2.h:59: rb = _mm_and_si128(mask, rb);
On 2014/11/14 14:22:58, mtklein wrote:
> Hmm.  It'd be nice to explain / assert this for readability.  Perhaps,
>   static inline void assert_eq(__m128i a, __m128i b) {
>     // Compare each byte pair for equality, then check all comparison bits are
> true.
>     SkASSERT(0xffff == _mm_movemask_epi8(_mm_cmpeq_epi8(a, b)));
>   }
> 
> Then assert these masks are unnecessary:
>   assert_eq(ag, _mm_and_sli128(mask, ag));  // _mm_srli_epi16(c,8) just did
this
> for us.
>   ...
>   assert_eq(rb, _mm_and_sli128(mask, rb));   // rb = _mm_srli_epi16(rb, 8) did
> this for us.
> 
> I'm assuming that the arguments to assert_eq will not even be evaluated in
> Release mode where SkASSERT() is a no-op.  If that's not the case, we could of
> course write assert_eq as a macro:
>   #define ASSERT_EQ(a,b) SkASSERT(0xffff ==
> _mm_movemask_epi8(_mm_cmpeq_epi8((a), (b))))

Done.

[mtklein, 2014-11-14 15:46:29]

lgtm, with one request before landing

https://codereview.chromium.org/724333003/diff/20001/src/opts/SkColor_opts_SS...
File src/opts/SkColor_opts_SSE2.h (right):

https://codereview.chromium.org/724333003/diff/20001/src/opts/SkColor_opts_SS...
src/opts/SkColor_opts_SSE2.h:187: 
Let's add #undef ASSERT_EQ here?  Good idea to keep good hygiene in headers.

[qiankun, 2014-11-14 16:03:22]

https://codereview.chromium.org/724333003/diff/20001/src/opts/SkColor_opts_SS...
File src/opts/SkColor_opts_SSE2.h (right):

https://codereview.chromium.org/724333003/diff/20001/src/opts/SkColor_opts_SS...
src/opts/SkColor_opts_SSE2.h:187: 
On 2014/11/14 15:46:29, mtklein wrote:
> Let's add #undef ASSERT_EQ here?  Good idea to keep good hygiene in headers.

Done.

[mtklein, 2014-11-14 16:05:09]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-11-14 16:06:14]

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

[commit-bot: I haz the power, 2014-11-14 16:23:58]

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