Implemented SSE version of ClampX_ClampY_{no}filter_scale

Implemented a SSE2 version of ClampX_ClampY_{no}filter_scale. Added two bitmap benchmarks to cover them in the bench. 
The original version:
out\Release\bench -config 8888 -match bitmap_clamp -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] bitmap_clamp_scale_filtering_8888_update_volatile  8888:
 cmsecs =  48.28
running bench [640 480] bitmap_clamp_scale_8888_update_volatile  8888: cmsecs =
 12.56

The SSE2 version:
out\Release\bench -config 8888 -match bitmap_clamp -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] bitmap_clamp_scale_filtering_8888_update_volatile  8888:
 cmsecs =  39.94
running bench [640 480] bitmap_clamp_scale_8888_update_volatile  8888: cmsecs =
 10.22

We can observe 17.27% gain with filter drawBitmap, and 18.63% gain with no filter drawBitmap.

With this patch, my HTML5 benchmark Myalbum Zoom can get 5% performance gain.

Run tests:
tests
gm
bench

[Jin A.Yang, 2012-02-21 05:04:46]

upload patch set 2

[Jin A.Yang, 2012-02-21 05:05:27]

patch2

[Jin A.Yang, 2012-02-21 05:18:43]

patch2

[Jin A.Yang, 2012-02-21 09:41:27]

Hi Tom,

Please help review this SSE patch, thanks!

Regards,
Jin

[TomH, 2012-02-21 12:26:57]

Jin,

I'll get into a detailed review later. Can you tell us / document how your new
benchmarks are different than using the command-line arguments "-scale
-forceFilter 1"?

[Jin A.Yang, 2012-02-21 13:16:05]

On 2012/02/21 12:26:57, TomH wrote:
> Jin,
> 
> I'll get into a detailed review later. Can you tell us / document how your new
> benchmarks are different than using the command-line arguments "-scale
> -forceFilter 1"?
I didn't notice these arguments when I try to measure the SSE performance. Just
now I have tried this command-line argument, and I got the same effect. So
please ignore the benchmark change when you review the code, it is not needed
now.
With the command line, I have the below data which show the SSE performance
gain:
filter mode:
bitmap_8888_update          9.6%     
bitmap_8888_update_volatile 8.3% 
bitmap_8888                 13% 
bitmap_8888_A               8%

nofilter mode:
bitmap_8888_update          8%     
bitmap_8888_update_volatile 12.6% 
bitmap_8888                 19.6% 
bitmap_8888_A               20.65%

[TomH, 2012-02-21 18:08:45]

For the most part LGTM.
Lines 243 and 255 are more-or-less nits; let's settle those and then I'll check
it in.

http://codereview.appspot.com/5685055/diff/1003/src/opts/SkBitmapProcState_op...
File src/opts/SkBitmapProcState_opts_SSE2.cpp (right):

http://codereview.appspot.com/5685055/diff/1003/src/opts/SkBitmapProcState_op...
src/opts/SkBitmapProcState_opts_SSE2.cpp:243: #define CHECK_FOR_DECAL
Why do we have this here? And why might we want to compile it out? If it's
something you considered during development and found to be a speedup, let's
make it always on and remove the #define/#ifdefs.

http://codereview.appspot.com/5685055/diff/1003/src/opts/SkBitmapProcState_op...
src/opts/SkBitmapProcState_opts_SSE2.cpp:255: const SkFixed one = s.fFilterOneX;
Is there a better name for this? Particularly when I see _mm_set1_epi32(one) vs
_mm_set1_epi32(1), it's not obvious what the variable means.

http://codereview.appspot.com/5685055/diff/1003/src/opts/SkBitmapProcState_op...
src/opts/SkBitmapProcState_opts_SSE2.cpp:262: const SkFixed fy =
SkScalarToFixed(pt.fY) - (s.fFilterOneY >> 1);
Oddly, it looks like SkScalarToFixed() isn't being inlined?  We can address that
here in a bug.

[Jin A.Yang, 2012-02-22 02:51:52]

http://codereview.appspot.com/5685055/diff/1003/src/opts/SkBitmapProcState_op...
File src/opts/SkBitmapProcState_opts_SSE2.cpp (right):

http://codereview.appspot.com/5685055/diff/1003/src/opts/SkBitmapProcState_op...
src/opts/SkBitmapProcState_opts_SSE2.cpp:243: #define CHECK_FOR_DECAL
On 2012/02/21 18:08:45, TomH wrote:
> Why do we have this here? And why might we want to compile it out? If it's
> something you considered during development and found to be a speedup, let's
> make it always on and remove the #define/#ifdefs.
I just follow the portable version. The portable version defined this, the
original contributor should have used this to test which way is faster.
Obviously the check for decal way should be faster, so I remove the
#define/#ifdefs.

http://codereview.appspot.com/5685055/diff/1003/src/opts/SkBitmapProcState_op...
src/opts/SkBitmapProcState_opts_SSE2.cpp:255: const SkFixed one = s.fFilterOneX;
On 2012/02/21 18:08:45, TomH wrote:
> Is there a better name for this? Particularly when I see _mm_set1_epi32(one)
vs
> _mm_set1_epi32(1), it's not obvious what the variable means.
This also just copied the portable version. I rechecked the code, in
SkBitmapProcState_matrixProcs.cpp, it has:
        // clamp gets special version of filterOne
        fFilterOneX = SK_Fixed1;
        fFilterOneY = SK_Fixed1;
        return ClampX_ClampY_Procs[index];
It looks like s.fFilterOneX really means SK_Fixed1;

http://codereview.appspot.com/5685055/diff/1003/src/opts/SkBitmapProcState_op...
src/opts/SkBitmapProcState_opts_SSE2.cpp:262: const SkFixed fy =
SkScalarToFixed(pt.fY) - (s.fFilterOneY >> 1);
On 2012/02/21 18:08:45, TomH wrote:
> Oddly, it looks like SkScalarToFixed() isn't being inlined?  We can address
that
> here in a bug.
From the code, SkScalarToFixed is defined as a macro

[Jin A.Yang, 2012-02-22 02:52:37]

revised patch

[TomH, 2012-02-22 18:53:33]

I went ahead and committed a hybrid between your patch 2 and patch 3 as r3227;
see below for where I want to tweak the style. We can address that in a followup
patch if it's important.

Thanks again, Jin!

Please close this code review when you get a chance.

http://codereview.appspot.com/5685055/diff/4003/src/opts/SkBitmapProcState_op...
File src/opts/SkBitmapProcState_opts_SSE2.cpp (right):

http://codereview.appspot.com/5685055/diff/4003/src/opts/SkBitmapProcState_op...
src/opts/SkBitmapProcState_opts_SSE2.cpp:281: SkFixed dx4 = dx2 + dx2;
Could you find a case where this makes a discernable impact on performance? We
don't (the compiler seems to generate the same assembly code either way?), and
it seems like a break-even on legibility. It's a pattern that we're wary of
because 32b builds often suffer from heavy register pressure.

If you've got a particular reason for doing this, let's not duplicate the
definition of dx[234] in all the branches of the if(), but move it up around
line 255-265, and likewise in the nofilter function.

[Jin A.Yang, 2012-02-23 01:29:30]

On 2012/02/22 18:53:33, TomH wrote:
> I went ahead and committed a hybrid between your patch 2 and patch 3 as r3227;
> see below for where I want to tweak the style. We can address that in a
followup
> patch if it's important.
> 
> Thanks again, Jin!
> 
> Please close this code review when you get a chance.
> 
>
http://codereview.appspot.com/5685055/diff/4003/src/opts/SkBitmapProcState_op...
> File src/opts/SkBitmapProcState_opts_SSE2.cpp (right):
> 
>
http://codereview.appspot.com/5685055/diff/4003/src/opts/SkBitmapProcState_op...
> src/opts/SkBitmapProcState_opts_SSE2.cpp:281: SkFixed dx4 = dx2 + dx2;
> Could you find a case where this makes a discernable impact on performance? We
> don't (the compiler seems to generate the same assembly code either way?), and
> it seems like a break-even on legibility. It's a pattern that we're wary of
> because 32b builds often suffer from heavy register pressure.
> 
> If you've got a particular reason for doing this, let's not duplicate the
> definition of dx[234] in all the branches of the if(), but move it up around
> line 255-265, and likewise in the nofilter function.

I supposed this can save some multiply instructions. But as you mentioned, this
also give heavy register pressure on 32b machine. From my test, it seems I can't
observe the obvious performance impact. 
Thanks for the review!