SSE2 optimization for matrix computation when scaling bitmap

The optimization routines are decal_filter_scale and decal_nofilter_scale. Below command is what I used to get the improvement on Android platform. ./skia_bench -config 8888 -match bitmap_8888_scale -repeat 200 w/o these opts: D/skia ( 1803): running bench [640 480] bitmap_8888_scale D/skia ( 1803): 8888: cmsecs = 55.02 D/skia ( 1803): running bench [640 480] bitmap_8888_scale_filter D/skia ( 1803): 8888: cmsecs = 162.63 w/ these opts: D/skia ( 4947): running bench [640 480] bitmap_8888_scale D/skia ( 4947): 8888: cmsecs = 53.40 D/skia ( 4947): running bench [640 480] bitmap_8888_scale_filter D/skia ( 4947): 8888: cmsecs = 157.67 Actually the benefits are not significant. I ran several times and the result is stable. The net gain is about 3%

[reed1, 2012-06-21 17:34:57]

The changes in the .cpp are pretty clear (though I can't read those intrinsics yet). However, do we have to copy-paste the entire .h file just to optimize one routines. I would like us to find a more scalable, maintainable technique. The problem is (as I found earlier when I introduced SkFractionalInt) that if we find a bug or change, we end up having to replicate the fix/change in multiple copies of the same routine.

[Lei, 2012-07-02 10:38:38]

Hi reed1, thanks for your suggestion. Actually, at the beginning, I also wanted to add the optimized routine in the original header file. Then I found that the NEON version created a separate file (SkBitmapProcState_matrix_clamp.h). The NEON routines are big change, though. I will add the single x86 routine in the original header file (SkBitmapProcState_matrix.h). If more routines are optimized in the future, I will try to figure out if there is better approach.

[Lei, 2012-07-02 10:38:54]

updated patch

[reed1, 2012-07-02 14:38:23]

I still don't see who sets USE_SSE2 (it is not automatically set when I test for it on my mac). I've justed add SK_CPU_SSE_LEVEL=2,3,41,42 to SkPreConfig. Can you use that define instead?

[Lei, 2012-07-03 01:46:27]

Sure. I will use this flag.

[Lei, 2012-07-03 01:47:12]

updated the patch after changing the flag from USE_SSE2 to SK_CPU_SSE_LEVEL

[reed1, 2012-07-03 12:10:49]

Sorry, the names have been churning lately. I think the right pattern for SSE2 is #if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2 ... #endif ... if SK_CPU_SSE_LEVEL is defined, it will be defined as some numeric value. To test for SSE2, you need to compare that with the symbolic constant for that level: SK_CPU_SSE_LEVEL_SSE2

[Lei, 2012-07-04 04:17:47]

Sure. I understand and saw the definitions in SkPreConfig.h. I will update the patch.

[Lei, 2012-07-04 04:19:04]

changed the SSE2 check condition at compile time

[Lei, 2012-07-04 04:21:07]

On 2012/07/04 04:19:04, Lei wrote: > changed the SSE2 check condition at compile time sorry, it's not latest and has minor error, will update

[Lei, 2012-07-04 04:21:41]

updated SSE2 check condition at compile time

[reed1, 2012-07-09 13:36:23]

Lei, the code looks good. Can you include any bench results that demonstrate that we get a speed-win with these changes?

[Lei, 2012-07-16 07:20:35]

Hi Reed, thanks. Actually, we previously did these optimizations based on Android platform. We ran the workload or benchmark, got the hotspots, and then tried to optimize it with SSE. We have the speedup for some workloads. Some is designed by our OEMs. I am not sure if I can mention the benchmark name. Anyway, I will summarize the benefits and update the commit message later.

[reed1, 2012-07-16 14:09:41]

Summarizing external benchmarks is good, but what the code-base really needs are benchmarks (either existing or new ones) that *we* can run before and after, and later on, to see that we don't regress performance with subsequent changes.

[Lei, 2012-07-17 08:00:01]

Hi Reed, thanks for your clarify. I understand your concern. I saw a little bit skia_bench. So I need to demonstrate the benefit of the existing skia_bench from these optimizations. If not, I need to create extra bench test and then commit the micro bench. If I need to do other things, please tell me. Thanks~

[reed1, 2012-07-17 12:16:18]

You summarized very well -- anytime we add assembly or other complexity to Skia, we want a unittest or benchmark or some other automatic test to verify 1. that the new code is correct, and stays correct 2. that the feature or optimization has a net performance gain

[Lei, 2012-07-25 03:22:46]

After detailed check, it seems that there are already one optimized routine for ClampX_ClampY_filter_scale. That is for Win32 while mine is for Android. I will compare the performance and then decide whether to check in or not. First, I will show the benefit of decal_filter_scale and decal_nofilter_scale.

[Lei, 2012-08-23 08:10:26]

only include matrix optimizations

[Lei, 2012-08-23 08:11:19]

and add the extra skia micro bench to demonstrate the benefit of decal_nofilter_scale

[Lei, 2012-08-23 08:14:26]

Actually these two routines also have NEON version. However, they are in Android tree not in SKIA upstream, possibly because that NEON optimizations are dedicated for Android devices. Most are ARM-based.

[Lei, 2012-08-27 02:03:15]

Hi Reed, please help review if you get a chance. Thanks very much!

[reed1, 2012-08-27 12:51:04]

Derek, are we missing some NEON code in android but not in skia?

[DerekS, 2012-08-27 13:00:53]

On 2012/08/27 12:51:04, reed1 wrote: > Derek, are we missing some NEON code in android but not in skia? We do have NEON optimized routines for decal_filter_scale and decal_nofilter_scale in the Skia tree. They have been moved from this file to http://code.google.com/p/skia/source/browse/trunk/src/opts/SkBitmapProcState_.... The next update I make to Android will reflect this file change.

[reed1, 2012-08-27 13:28:34]

LGTM

[Lei, 2012-08-28 07:33:47]

Hi DerekS, very sorry. Before I haven't realized the structure differences of MatrixProcs between the upstream and Android SKIA. I understand that the motivation of the change is to move all the specific platform optimizations into src/opts directory. So I will follow your change to add the SSE routines of MatrixProcs. One minor issue is that the optimized routines of decal_filter_scale and decal_nofilter_scale are both used by ClampXY, RepeatXY and GeneralXY. Right now, it seems that they are just used by ClampXY and RepeatXY.

[DerekS, 2012-08-28 13:31:25]

I'm fine with keeping this code here for now as it should get inlined. https://codereview.appspot.com/6327043/diff/19001/src/core/SkBitmapProcState_... File src/core/SkBitmapProcState_matrixProcs.cpp (right): https://codereview.appspot.com/6327043/diff/19001/src/core/SkBitmapProcState_... src/core/SkBitmapProcState_matrixProcs.cpp:271: } don't you need an #endif here?

[Lei, 2012-08-29 02:29:47]

Hi DerekS, you are right. I am totally OK to follow the example you did or just keep the code here. I noticed that RepeatXY can also use SSE to optimize. I will submit one patch later. If there is no need to do the change, please help merge it. Thanks again, Reed and DerekS!

[reed1, 2012-08-29 19:27:41]

Lei, you have been granted Committer status for Skia. You should be able to check this in yourself. Let me or Elliot (on cc) know if you have any problems.