Implemented a SSE version of blit_lcd16

Implement a SSE version of blit_lcd16. In order to use the original version
for pixels which are not aligned in SSE version, move the original version to
SkColorPriv.h
Add 3 LCD bench cases to test the performance. Use text size larget to 48, then we can use
SSE mostly.

The 3 bench cases show good performance improvement:
original version of blit_lcd16:
out\Release\bench.exe -config 8888 -match text_48 -repeat 200
skia bench: alpha=0xFF antialias=1 filter=0 rotate=0 scale=0 clip=0 dither=defau
lt strokeWidth=none scalar=float system=WIN32
running bench [640 480]               text_48_LCD_88  8888: cmsecs =  31.67
running bench [640 480]               text_48_LCD_FF  8888: cmsecs =  26.52
running bench [640 480]               text_48_LCD_BK  8888: cmsecs =  26.68

SSE version of blit_lcd16:
out\Release\bench.exe -config 8888 -match text_48 -repeat 200
skia bench: alpha=0xFF antialias=1 filter=0 rotate=0 scale=0 clip=0 dither=defau
lt strokeWidth=none scalar=float system=WIN32
running bench [640 480]               text_48_LCD_88  8888: cmsecs =  20.12
running bench [640 480]               text_48_LCD_FF  8888: cmsecs =  18.25
running bench [640 480]               text_48_LCD_BK  8888: cmsecs =  18.49

From the data, we can see SSE version has about 30% performance improvement:
text_48_LCD_88 36.46%
text_48_LCD_FF 31.18%
text_48_LCD_BK 30.69%

BUG=451

[Jin A.Yang, 2012-02-03 07:30:56]

Hi Mike,

This is Jin from Intel, I have implemented a sse version of blit_lcd16, can you
please help review it? Thanks!

Regards,
Jin

[TomH, 2012-02-03 22:00:37]

Thank you very much for writing this. It was on my todo list, but I don't have
the grasp of SSE to do as good a job.

48-point text isn't a very relevant test case for us, but it looks like you've
given us a 15% savings on the 16-point benchmarks, which is great.
Unfortunately, profiling suggests that we're almost never hitting the SSE code,
but instead falling through the generic procs, since we're usually not blitting
8 aligned pixels at a time in text. 

Oddly enough, although you declared SkBlendLCD16Opaque inline (which I
questioned, since we need to pass around function pointers to it), VS2010 is
only intermittently inlining it - when we call it from the SSE2 procs, we're
getting an explicit call complete with stack setup and teardown which turns out
to be a significant cost; if we could convince the compiler to actually inline
it there without blowing the instruction caches we could save another 5-10%.

http://codereview.appspot.com/5617058/diff/1/bench/TextBench.cpp
File bench/TextBench.cpp (right):

http://codereview.appspot.com/5617058/diff/1/bench/TextBench.cpp#newcode148
bench/TextBench.cpp:148: static SkBenchmark* Fact33(void* p) { return new
TextBench(p, STR, 48, 0x88FF0000, kLCD); }
I believe that 48pt text is not a common case for us, or somewhere we're
particularly interested in optimizing; 8-16pt is much more relevant. I'm not
sure we should add these to the benchmark suite.

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:731: static inline SkPMColor SkBlendLCD16(int srcA,
int srcR, int srcG, int srcB, SkPMColor dst, uint16_t mask)
Like the original source, this function needs to take an additional SkPMColor
parameter to make all the types work out.
Nit: line length

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:739: */
Nit: indentation

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:748: 
Does srcA also need to be upscaled 255->256? If not, can we have a comment
documenting the fact that there's a reason we've got it in an int?

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:758: // to be opaque
We like to assert this kind of restriction. Something on the order of
SkASSERT(SkGetPackedA32(dst) == 0xFF)?

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:765: static inline SkPMColor SkBlendLCD16Opaque(int
srcR, int srcG, int srcB, SkPMColor dst, uint16_t mask, SkPMColor opaqueDst)
The meaning of this function isn't obvious from the name (or from reading the
code - you're special-casing srcA=255, but that doesn't explain opaqueDst).
Nit: line length.

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask.h
File src/core/SkBlitMask.h (right):

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask.h#newcode34
src/core/SkBlitMask.h:34: *  Function pointer that blits a row of mask(lcd16)
into a row of dst colorized by color.
I'm not sure what "colorized by color" means.
Nit: line length

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask_D32.cpp
File src/core/SkBlitMask_D32.cpp (right):

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask_D32.cpp#newc...
src/core/SkBlitMask_D32.cpp:74: return  SkBlitLCD16OpaqueRow;
Elsewhere you've explicitly declared these inline, which can do odd things when
you're returning function pointers. Did they really need to be inlined?

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask_D32.cpp#newc...
src/core/SkBlitMask_D32.cpp:76: return  SkBlitLCD16Row;
Although VS2010 doesn't complain, coercing these two functions to the same
pointer type is wrong - adding the extra ignored parameter will fix that.

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp
File src/opts/SkBlitRow_opts_SSE2.cpp (right):

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:491: __m128i b =
_mm_and_si128(_mm_slli_epi32(mask, SK_B16_BITS-5), _mm_set1_epi32(0x0000001F));
Nit: line length throughout this file.

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:600: __m128i mask_pixel =
_mm_loadu_si128(mask);
The comments above these lines aren't very revealing. More useful would be
explaining the comparisons to FFFF/FF/FF00 below, which aren't obviously
motivated.

[Jin A.Yang, 2012-02-07 06:39:41]

http://codereview.appspot.com/5617058/diff/1/bench/TextBench.cpp
File bench/TextBench.cpp (right):

http://codereview.appspot.com/5617058/diff/1/bench/TextBench.cpp#newcode148
bench/TextBench.cpp:148: static SkBenchmark* Fact33(void* p) { return new
TextBench(p, STR, 48, 0x88FF0000, kLCD); }
On 2012/02/03 22:00:37, TomH wrote:
> I believe that 48pt text is not a common case for us, or somewhere we're
> particularly interested in optimizing; 8-16pt is much more relevant. I'm not
> sure we should add these to the benchmark suite.

I tested it with 16pt on windows, it will not enter blit_lcd16 cod path, because
in windows SkScalerContext_Windows::generateImage it will check whether the
glyph is BW, for 16pt, it is really BW and call rgb_to_bw. While for 48pt case,
it will call rgb_to_lcd16.
That's why you see it not call sse when profile 16pt case. So I add 48pt cases
to check the performance improvement.
Under Linux, it will not do this check and will go into our SSE code, but most
times the width is less than 8, will go to the original version. Not much
improvement for 16pt. In my patch 2, I change the SSE code to only process 4
pixels at one time, then we can use SSE more for 16pt case in Linux.
So I removed the 48pt cases in patch 2.

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:731: static inline SkPMColor SkBlendLCD16(int srcA,
int srcR, int srcG, int srcB, SkPMColor dst, uint16_t mask)
On 2012/02/03 22:00:37, TomH wrote:
> Like the original source, this function needs to take an additional SkPMColor
> parameter to make all the types work out.
> Nit: line length
This function does not need the additional parameter because it will not be
assigned to function pointer. What your are saying should be SkBlitLCD16Row

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:739: */
On 2012/02/03 22:00:37, TomH wrote:
> Nit: indentation

Done.

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:748: 
On 2012/02/03 22:00:37, TomH wrote:
> Does srcA also need to be upscaled 255->256? If not, can we have a comment
> documenting the fact that there's a reason we've got it in an int?
srcA has been upscaled to 256 before call this function. see line 806. Add a
comment to document this.
Done

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:758: // to be opaque
On 2012/02/03 22:00:37, TomH wrote:
> We like to assert this kind of restriction. Something on the order of
> SkASSERT(SkGetPackedA32(dst) == 0xFF)?
I think we do not need to assert this, because from the comment We can see the
dst is going to be opaque while not currently be.

http://codereview.appspot.com/5617058/diff/1/include/core/SkColorPriv.h#newco...
include/core/SkColorPriv.h:765: static inline SkPMColor SkBlendLCD16Opaque(int
srcR, int srcG, int srcB, SkPMColor dst, uint16_t mask, SkPMColor opaqueDst)
On 2012/02/03 22:00:37, TomH wrote:
> The meaning of this function isn't obvious from the name (or from reading the
> code - you're special-casing srcA=255, but that doesn't explain opaqueDst).
> Nit: line length.

opaqueDst is the valuse to assign when mask is FFFF.

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask.h
File src/core/SkBlitMask.h (right):

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask.h#newcode34
src/core/SkBlitMask.h:34: *  Function pointer that blits a row of mask(lcd16)
into a row of dst colorized by color.
On 2012/02/03 22:00:37, TomH wrote:
> I'm not sure what "colorized by color" means.
> Nit: line length
Just follow line 25's explanation of ColorProc, maybe "colorized by a single
color" will be more meaningful.
Done

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask_D32.cpp
File src/core/SkBlitMask_D32.cpp (right):

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask_D32.cpp#newc...
src/core/SkBlitMask_D32.cpp:74: return  SkBlitLCD16OpaqueRow;
On 2012/02/03 22:00:37, TomH wrote:
> Elsewhere you've explicitly declared these inline, which can do odd things
when
> you're returning function pointers. Did they really need to be inlined?
From MSDN, if the function will be assigned to function pointer, then it will
not be inlined. So I have remove the inline declaration in my patch2.

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask_D32.cpp#newc...
src/core/SkBlitMask_D32.cpp:76: return  SkBlitLCD16Row;
On 2012/02/03 22:00:37, TomH wrote:
> Although VS2010 doesn't complain, coercing these two functions to the same
> pointer type is wrong - adding the extra ignored parameter will fix that.
It do have the extra ignored parameter.

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp
File src/opts/SkBlitRow_opts_SSE2.cpp (right):

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:491: __m128i b =
_mm_and_si128(_mm_slli_epi32(mask, SK_B16_BITS-5), _mm_set1_epi32(0x0000001F));
On 2012/02/03 22:00:37, TomH wrote:
> Nit: line length throughout this file.

Done.

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:600: __m128i mask_pixel =
_mm_loadu_si128(mask);
On 2012/02/03 22:00:37, TomH wrote:
> The comments above these lines aren't very revealing. More useful would be
> explaining the comparisons to FFFF/FF/FF00 below, which aren't obviously
> motivated.

Done.

[Jin A.Yang, 2012-02-07 06:40:20]

patch set2

[Jin A.Yang, 2012-02-07 06:57:31]

Thanks for your review, I have made some changes according to your comments and
uploaded the patch set 2.
I have changed the SSE code from processing 8 pixels one time to 4 pixels as to
let 16pt case(it has mostly less than 8 pixel wide) to benefit from the SSE.
After this change, the performance improvement seems pretty good, I tested on my
ubuntu:
original version:
out\Release\bench.exe -config 8888 -match text_16_LCD -repeat 200
00
skia bench: alpha=0xFF antialias=1 filter=0 rotate=0 scale=0 clip=0 dither=defau
lt strokeWidth=none scalar=float system=UNIX
running bench [640 480]               text_16_LCD_88  8888: cmsecs =   5.91
running bench [640 480]               text_16_LCD_FF  8888: cmsecs =   5.18
running bench [640 480]               text_16_LCD_BK  8888: cmsecs =   5.15

SSE version:
out\Release\bench.exe -config 8888 -match text_16_LCD -repeat 200
00
skia bench: alpha=0xFF antialias=1 filter=0 rotate=0 scale=0 clip=0 dither=defau
lt strokeWidth=none scalar=float system=UNIX
running bench [640 480]               text_16_LCD_88  8888: cmsecs =   4.63
running bench [640 480]               text_16_LCD_FF  8888: cmsecs =   4.09
running bench [640 480]               text_16_LCD_BK  8888: cmsecs =   4.06

The improvements are:
text_16_LCD_88  21.66% 
text_16_LCD_FF  21.04%
text_16_LCD_BK  21.17%

For 48pt case, we can get ~47% improvement.

on windows, I can't test 16pt case because on windows it will convert rgb to bw
mask format while not lcd16. But for the 48pt case I also can observe 45%+
performance gain.

[reed1, 2012-02-07 13:55:42]

What is the smallest size that doesn't convert to BW? 24? 32? The sizes in bench
were chosen a bit arbitrarily, in an attempt to match what we see on web pages.
48 is fine, though it seems a lot larger than what we see on the web.

[TomH, 2012-02-07 16:47:02]

On 2012/02/07 06:57:31, Jin Yang wrote:
> After this change, the performance improvement seems pretty good, I tested on
my
> ubuntu:

Great! I'm glad to see that reducing unrolling helped - we find that same
counter-intuitive fact much more often than I expect.

> on windows, I can't test 16pt case because on windows it will convert rgb to
bw
> mask format while not lcd16.

So, when I use your patch on my Win7 machine, profiling clearly shows this to be
incorrect: we spend a lot of time in SkBlitLCD16{Opaque}Row_SSE2 when we run the
16pt text benchmarks, and we're taking the SSE-accelerated path as well as the
edge cases. Raw performance also suggests that we're hitting the new code.

Top-of-tree skia:
running bench [640 480]               text_16_LCD_88  8888: cmsecs =  10.92
running bench [640 480]               text_16_LCD_FF  8888: cmsecs =  11.96
running bench [640 480]               text_16_LCD_BK  8888: cmsecs =  11.96

With SSE:
running bench [640 480]               text_16_LCD_88  8888: cmsecs =   8.63
running bench [640 480]               text_16_LCD_FF  8888: cmsecs =   8.53
running bench [640 480]               text_16_LCD_BK  8888: cmsecs =   8.42

I'm very happy with those numbers, but I need to understand why this is
happening when you're stating that it shouldn't. Do you have any idea what the
mechanism could be?

[TomH, 2012-02-07 20:32:41]

Mike,

I forgot that rietveld ate my nitty question to you: I don't think the function
names added to SkBlitRow_opts_SSE2.h are consistent with those already there,
but the naming convention isn't clear.

[Jin A.Yang, 2012-02-08 01:54:08]

On 2012/02/07 13:55:42, reed1 wrote:
> What is the smallest size that doesn't convert to BW? 24? 32? The sizes in
bench
> were chosen a bit arbitrarily, in an attempt to match what we see on web
pages.
> 48 is fine, though it seems a lot larger than what we see on the web.

I tested on my Win7, 18pt is the smallest size that doesn't covert to BW.

[Jin A.Yang, 2012-02-08 02:00:39]

On 2012/02/07 16:47:02, TomH wrote:
> On 2012/02/07 06:57:31, Jin Yang wrote:
> > After this change, the performance improvement seems pretty good, I tested
on
> my
> > ubuntu:
> 
> Great! I'm glad to see that reducing unrolling helped - we find that same
> counter-intuitive fact much more often than I expect.
> 
> > on windows, I can't test 16pt case because on windows it will convert rgb to
> bw
> > mask format while not lcd16.
> 
> So, when I use your patch on my Win7 machine, profiling clearly shows this to
be
> incorrect: we spend a lot of time in SkBlitLCD16{Opaque}Row_SSE2 when we run
the
> 16pt text benchmarks, and we're taking the SSE-accelerated path as well as the
> edge cases. Raw performance also suggests that we're hitting the new code.
> 
> Top-of-tree skia:
> running bench [640 480]               text_16_LCD_88  8888: cmsecs =  10.92
> running bench [640 480]               text_16_LCD_FF  8888: cmsecs =  11.96
> running bench [640 480]               text_16_LCD_BK  8888: cmsecs =  11.96
> 
> With SSE:
> running bench [640 480]               text_16_LCD_88  8888: cmsecs =   8.63
> running bench [640 480]               text_16_LCD_FF  8888: cmsecs =   8.53
> running bench [640 480]               text_16_LCD_BK  8888: cmsecs =   8.42
> 
> I'm very happy with those numbers, but I need to understand why this is
> happening when you're stating that it shouldn't. Do you have any idea what the
> mechanism could be?

I remember in the patch set 1, you also encounter the problem we never enter SSE
code on windows.
In my win7, 16pt will be converted to BW. You can debug the function
is_rgb_really_bw in SkFontHost_win.cpp. It seems the font image is generated in
function HDCOffscreen::draw. Should it be related to system resolution or font
config? I am not sure.

[Jin A.Yang, 2012-02-10 12:28:27]

any update?

[TomH, 2012-02-10 16:17:44]

On 2012/02/10 12:28:27, Jin Yang wrote:
> any update?

We can consistently see 16-point LCD text on our Win 7 machines, and
is_rgb_really_bw() is returning false there. We believe there's a user/OS
setting that controls whether GDI uses LCD at all and for what font sizes, and
our best guess is that you've got the LCD point threshold set high enough you're
getting different results. I've filed a bug to try to disable that setting when
we're doing LCD tests in bench & gm
(http://code.google.com/p/skia/issues/detail?id=483), but we'll stop worrying
about the mismatch between your results and mine.

I'll do a more detailed code review and plan to get back to you in a couple of
hours. If the code is good, I'll go ahead and commit it.

[TomH, 2012-02-10 18:54:49]

I'd like to see my questions at src/opts/SkBlitRow_opts_SSE2.cpp:603 addressed;
everything else I can take care of if I do the commit (assuming you don't have
write access to the Skia repo and need one of us to commit for you)?

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask.h
File src/core/SkBlitMask.h (right):

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask.h#newcode34
src/core/SkBlitMask.h:34: *  Function pointer that blits a row of mask(lcd16)
into a row of dst colorized by color.
On 2012/02/07 06:39:41, Jin Yang wrote:
> On 2012/02/03 22:00:37, TomH wrote:
> > I'm not sure what "colorized by color" means.
> > Nit: line length
> Just follow line 25's explanation of ColorProc, maybe "colorized by a single
> color" will be more meaningful.

Not 'Done'. I think the original comment is bad, but that's something we don't
have to fix in this CL, I suppose.

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask_D32.cpp
File src/core/SkBlitMask_D32.cpp (right):

http://codereview.appspot.com/5617058/diff/1/src/core/SkBlitMask_D32.cpp#newc...
src/core/SkBlitMask_D32.cpp:76: return  SkBlitLCD16Row;
On 2012/02/07 06:39:41, Jin Yang wrote:
> On 2012/02/03 22:00:37, TomH wrote:
> > Although VS2010 doesn't complain, coercing these two functions to the same
> > pointer type is wrong - adding the extra ignored parameter will fix that.
> It do have the extra ignored parameter.

Oops, you're right - I was looking at the Blend function, not the Blit function.

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp
File src/opts/SkBlitRow_opts_SSE2.cpp (right):

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:491: __m128i b =
_mm_and_si128(_mm_slli_epi32(mask, SK_B16_BITS-5), _mm_set1_epi32(0x0000001F));
On 2012/02/07 06:39:41, Jin Yang wrote:
> On 2012/02/03 22:00:37, TomH wrote:
> > Nit: line length throughout this file.
> 
> Done.

Nit: Unless there's a macro definition going on, we don't put the backslashes -
but I can fix that on commit.

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:600: __m128i mask_pixel =
_mm_loadu_si128(mask);
On 2012/02/07 06:39:41, Jin Yang wrote:
> On 2012/02/03 22:00:37, TomH wrote:
> > The comments above these lines aren't very revealing. More useful would be
> > explaining the comparisons to FFFF/FF/FF00 below, which aren't obviously
> > motivated.
> 
> Done.

Only half-done. These are still unuseful comments. They're focused on *what* is
being done, and give no context as to *why*. _mm instructions are unusual, not
standard C++, so some documentation of *what* might be called for - but that'd
be far more important for something complex like pack_cmp than for dst_pixel.

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:603: if (pack_cmp != 0xFFFF) {
We changed from 8 pixels to 4 pixels - should this mask still be 0xFFFF?

Also, the comment here is appreciated, but it doesn't fit the code - you're
checking to see if the mask pixels are all 1, but the comment is talking about
the all-0 case.

http://codereview.appspot.com/5617058/diff/12001/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

http://codereview.appspot.com/5617058/diff/12001/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:769: SkPMColor opaqueDst)
Nit: alignment; I can fix when I commit.

[Jin A.Yang, 2012-02-13 02:50:24]

patch set 3

[Jin A.Yang, 2012-02-13 02:56:10]

Thanks for your patient review, I uploaded the patch set3.
Please help to commit the code if this version is proper to commit or you can
fix the issues when you commit. Thanks!

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp
File src/opts/SkBlitRow_opts_SSE2.cpp (right):

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:491: __m128i b =
_mm_and_si128(_mm_slli_epi32(mask, SK_B16_BITS-5), _mm_set1_epi32(0x0000001F));
On 2012/02/10 18:54:49, TomH wrote:
> On 2012/02/07 06:39:41, Jin Yang wrote:
> > On 2012/02/03 22:00:37, TomH wrote:
> > > Nit: line length throughout this file.
> > 
> > Done.
> 
> Nit: Unless there's a macro definition going on, we don't put the backslashes
-
> but I can fix that on commit.

Done.

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:600: __m128i mask_pixel =
_mm_loadu_si128(mask);
On 2012/02/10 18:54:49, TomH wrote:
> On 2012/02/07 06:39:41, Jin Yang wrote:
> > On 2012/02/03 22:00:37, TomH wrote:
> > > The comments above these lines aren't very revealing. More useful would be
> > > explaining the comparisons to FFFF/FF/FF00 below, which aren't obviously
> > > motivated.
> > 
> > Done.
> 
> Only half-done. These are still unuseful comments. They're focused on *what*
is
> being done, and give no context as to *why*. _mm instructions are unusual, not
> standard C++, so some documentation of *what* might be called for - but that'd
> be far more important for something complex like pack_cmp than for dst_pixel.

Done.

http://codereview.appspot.com/5617058/diff/1/src/opts/SkBlitRow_opts_SSE2.cpp...
src/opts/SkBlitRow_opts_SSE2.cpp:603: if (pack_cmp != 0xFFFF) {
On 2012/02/10 18:54:49, TomH wrote:
> We changed from 8 pixels to 4 pixels - should this mask still be 0xFFFF?
> 
> Also, the comment here is appreciated, but it doesn't fit the code - you're
> checking to see if the mask pixels are all 1, but the comment is talking about
> the all-0 case.
_mm_loadl_epi64 will load 4 pixels to the lower 64bit of XMM and fill the high
64bit with 0, so compare to 0 we will get FF of the high 64bit. Because we are
checking if all 0, then we can avoid to check it like this: if ((pack_cmp &
0x00FF)!= 0x00FF)

http://codereview.appspot.com/5617058/diff/12001/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

http://codereview.appspot.com/5617058/diff/12001/include/core/SkColorPriv.h#n...
include/core/SkColorPriv.h:769: SkPMColor opaqueDst)
On 2012/02/10 18:54:49, TomH wrote:
> Nit: alignment; I can fix when I commit.

Done.

[TomH, 2012-02-14 16:13:11]

This was committed to Skia in r3189.
We expect 25-30% speedup on Windows, 4-7% on Linux (less register pressure on
64b platforms), none on Mac (currently using 32b LCD text path instead of 16b).

Jin, please close this review when you get the opportunity.