Add SSSE3 acceleration for S32_D16_filter_DXDY

Add SSSE3 acceleration for S32_D16_filter_DXDY

With this CL, related nanobench can be improved for 565 config.
                bitmap_BGRA_8888_scale_rotate_bilerp     115us -> 70.5us       0.61x
bitmap_BGRA_8888_update_volatile_scale_rotate_bilerp     115us -> 70.5us       0.61x
         bitmap_BGRA_8888_update_scale_rotate_bilerp     112us ->   68us       0.6x


BUG=skia:

Committed: https://skia.googlesource.com/skia/+/45a05780867a06b9f8a8d5240cf6c5d5a2c15a35

Committed: https://skia.googlesource.com/skia/+/60f3c657cc0235650b630be78105fc47d37385e7

[qiankun, 2014-12-02 10:51:41]

PTAL.

https://codereview.chromium.org/773753002/diff/1/src/opts/SkBitmapProcState_o...
File src/opts/SkBitmapProcState_opts_SSSE3.cpp (right):

https://codereview.chromium.org/773753002/diff/1/src/opts/SkBitmapProcState_o...
src/opts/SkBitmapProcState_opts_SSSE3.cpp:738: SkAutoTMalloc<uint32_t>
colors32(count);
If "uint32_t colors32[128];" used here, it's more faster. But the array size
depends on count. In current code, count will not exceed 64. The data as
follows:
         bitmap_BGRA_8888_update_scale_rotate_bilerp     112us -> 72.3us       
0.64x
bitmap_BGRA_8888_update_volatile_scale_rotate_bilerp     115us ->   67us       
0.58x
                bitmap_BGRA_8888_scale_rotate_bilerp     115us -> 66.9us       
0.58x

[qiankun, 2014-12-02 10:52:44]

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

[qiankun, 2014-12-02 10:52:45]

PTAL

[mtklein, 2014-12-02 15:19:05]

https://codereview.chromium.org/773753002/diff/1/src/opts/SkBitmapProcState_o...
File src/opts/SkBitmapProcState_opts_SSSE3.cpp (right):

https://codereview.chromium.org/773753002/diff/1/src/opts/SkBitmapProcState_o...
src/opts/SkBitmapProcState_opts_SSSE3.cpp:738: SkAutoTMalloc<uint32_t>
colors32(count);
On 2014/12/02 10:51:41, qiankun wrote:
> If "uint32_t colors32[128];" used here, it's more faster. But the array size
> depends on count. In current code, count will not exceed 64. The data as
> follows:
>          bitmap_BGRA_8888_update_scale_rotate_bilerp     112us -> 72.3us      

> 0.64x
> bitmap_BGRA_8888_update_volatile_scale_rotate_bilerp     115us ->   67us      

> 0.58x
>                 bitmap_BGRA_8888_scale_rotate_bilerp     115us -> 66.9us      

> 0.58x

Might be worth trying SkAutoSTMalloc<64, uint32_t>, which should be both safe
and stay on the stack when count <= 64.

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

https://codereview.chromium.org/773753002/diff/1/src/opts/opts_check_x86.cpp#...
src/opts/opts_check_x86.cpp:180: fSampleProc16 = S32_D16_filter_DXDY_SSSE3;
We need to check supports_simd(SK_CPU_SSE_LEVEL_SSSE3).

Why don't we pull that out into a local constant so it's more obvious?

if (!supports_simd(SK_CPU_SSE_LEVEL_SSE2)) {
   return;
}
const bool ssse3 = supports_simd(SK_CPU_SSE_LEVEL_SSSE3);
...

} else if (ssse3 && fSampleProc16 == S32_D16_filter_DXDY) {
   fSampleProc16 = S32_D16_filter_DXDY_SSSE3;
}
...

[qiankun1, 2014-12-02 15:58:42]

qkmiao@gmail.com changed reviewers:
+ qkmiao@gmail.com

[qiankun1, 2014-12-02 15:58:43]

Thanks for comments.

https://codereview.chromium.org/773753002/diff/1/src/opts/SkBitmapProcState_o...
File src/opts/SkBitmapProcState_opts_SSSE3.cpp (right):

https://codereview.chromium.org/773753002/diff/1/src/opts/SkBitmapProcState_o...
src/opts/SkBitmapProcState_opts_SSSE3.cpp:738: SkAutoTMalloc<uint32_t>
colors32(count);
On 2014/12/02 15:19:05, mtklein wrote:
> On 2014/12/02 10:51:41, qiankun wrote:
> > If "uint32_t colors32[128];" used here, it's more faster. But the array size
> > depends on count. In current code, count will not exceed 64. The data as
> > follows:
> >          bitmap_BGRA_8888_update_scale_rotate_bilerp     112us -> 72.3us    
 
> 
> > 0.64x
> > bitmap_BGRA_8888_update_volatile_scale_rotate_bilerp     115us ->   67us    
 
> 
> > 0.58x
> >                 bitmap_BGRA_8888_scale_rotate_bilerp     115us -> 66.9us    
 
> 
> > 0.58x
> 
> Might be worth trying SkAutoSTMalloc<64, uint32_t>, which should be both safe
> and stay on the stack when count <= 64.

Done. Where is these performance improvement from? Just heap vs. stack?
Now the data are:
                bitmap_BGRA_8888_scale_rotate_bilerp     115us -> 70.5us       
0.61x
bitmap_BGRA_8888_update_volatile_scale_rotate_bilerp     115us -> 70.5us       
0.61x
         bitmap_BGRA_8888_update_scale_rotate_bilerp     112us ->   68us       
0.6x

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

https://codereview.chromium.org/773753002/diff/1/src/opts/opts_check_x86.cpp#...
src/opts/opts_check_x86.cpp:180: fSampleProc16 = S32_D16_filter_DXDY_SSSE3;
On 2014/12/02 15:19:05, mtklein wrote:
> We need to check supports_simd(SK_CPU_SSE_LEVEL_SSSE3).
> 
> Why don't we pull that out into a local constant so it's more obvious?
> 
> if (!supports_simd(SK_CPU_SSE_LEVEL_SSE2)) {
>    return;
> }
> const bool ssse3 = supports_simd(SK_CPU_SSE_LEVEL_SSSE3);
> ...
> 
> } else if (ssse3 && fSampleProc16 == S32_D16_filter_DXDY) {
>    fSampleProc16 = S32_D16_filter_DXDY_SSSE3;
> }
> ...

Done.

[mtklein, 2014-12-02 16:52:22]

lgtm

https://codereview.chromium.org/773753002/diff/1/src/opts/SkBitmapProcState_o...
File src/opts/SkBitmapProcState_opts_SSSE3.cpp (right):

https://codereview.chromium.org/773753002/diff/1/src/opts/SkBitmapProcState_o...
src/opts/SkBitmapProcState_opts_SSSE3.cpp:738: SkAutoTMalloc<uint32_t>
colors32(count);
On 2014/12/02 15:58:43, qiankun1 wrote:
> On 2014/12/02 15:19:05, mtklein wrote:
> > On 2014/12/02 10:51:41, qiankun wrote:
> > > If "uint32_t colors32[128];" used here, it's more faster. But the array
size
> > > depends on count. In current code, count will not exceed 64. The data as
> > > follows:
> > >          bitmap_BGRA_8888_update_scale_rotate_bilerp     112us -> 72.3us  
 
>  
> > 
> > > 0.64x
> > > bitmap_BGRA_8888_update_volatile_scale_rotate_bilerp     115us ->   67us  
 
>  
> > 
> > > 0.58x
> > >                 bitmap_BGRA_8888_scale_rotate_bilerp     115us -> 66.9us  
 
>  
> > 
> > > 0.58x
> > 
> > Might be worth trying SkAutoSTMalloc<64, uint32_t>, which should be both
safe
> > and stay on the stack when count <= 64.
> 
> Done. Where is these performance improvement from? Just heap vs. stack?
> Now the data are:
>                 bitmap_BGRA_8888_scale_rotate_bilerp     115us -> 70.5us      

> 0.61x
> bitmap_BGRA_8888_update_volatile_scale_rotate_bilerp     115us -> 70.5us      

> 0.61x
>          bitmap_BGRA_8888_update_scale_rotate_bilerp     112us ->   68us      

> 0.6x

Right, just heap vs. stack.  There's no difference between heap memory and stack
memory, except
  - the stack usually has better cache locality, and
  - allocating on the stack is essentially free compared to calling malloc/free.

When we have no statically provable upper bound, but do have a practical upper
bound, we find it's generally a pretty big win to allocate that practical upper
bound on the stack and fall back to the heap only in exceptional cases.

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

https://codereview.chromium.org/773753002/diff/20001/src/opts/opts_check_x86....
src/opts/opts_check_x86.cpp:156: if (supports_simd(SK_CPU_SSE_LEVEL_SSSE3)) {
Mind replacing all these other SSSE3 checks with  "if (ssse3)" too?

[qiankun, 2014-12-03 02:15:53]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2014-12-03 02:16:12]

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

[commit-bot: I haz the power, 2014-12-03 02:26:14]

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

[jam, 2014-12-03 23:19:01]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/761103003/ by jam@chromium.org.

The reason for reverting is: breaks build when not using SSE3, since the two
method definitions differ in parameter types (typo).

[qiankun, 2014-12-04 02:03:59]

On 2014/12/03 23:19:01, jam wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/761103003/ by mailto:jam@chromium.org.
> 
> The reason for reverting is: breaks build when not using SSE3, since the two
> method definitions differ in parameter types (typo).

Sorry. I fixed this build failure.

[reed1, 2014-12-04 13:48:08]

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

[reed1, 2014-12-04 13:48:08]

The skia build-bots didn't catch this I presume .. can we change that?

[mtklein, 2014-12-04 14:15:27]

On 2014/12/04 13:48:08, reed1 wrote:
> The skia build-bots didn't catch this I presume .. can we change that?

Yes, but not in this CL.  We need at least one new builder bot that uses or
pretends to use a very incapable toolchain.  We'd have to _not_ pass -msse2 /
-mssse3 / etc. when building the _SSE2.cpp / _SSSE3.cpp files.  This bug was in
an area that exists only to support compilers that are not able to compile SSSE3
code.  This typically comes up when Mozilla's building on OpenBSD, and for
Android framework builds where we don't have fine control over which file gets
which -msseN flags.

[mtklein, 2014-12-04 14:17:00]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-12-04 14:17:38]

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

[commit-bot: I haz the power, 2014-12-04 14:20:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-04 14:20:54]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[borenet, 2014-12-04 14:23:30]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-12-04 14:23:46]

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

[borenet, 2014-12-04 14:23:56]

On 2014/12/04 14:20:54, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
>
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

Please ignore the flaky Windows trybot. It doesn't want to apply patches.

[commit-bot: I haz the power, 2014-12-04 14:27:08]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/60f3c657cc0235650b630be78105fc47d37385e7