Make SSE2/Neon convolution functions not to read extra bytes

Make SSE2/Neon convolution functions not to read extra bytes

This change makes SSE2/Neon horizontal convolution functions do not read
extra pixels past the end of the buffer. So we can remove all the SIMD 
specific logic in SkConvolver to deal with last couple of rows and also 
avoid applying padding to convolution filters.

Performance impact is small. Nanobench time change: 
                              SSE2    NEON
bitmap_scale_filter_64_256     1%     -2%
bitmap_scale_filter_256_64     1%      2%
bitmap_scale_filter_90_10      1%     -1%
bitmap_scale_filter_90_30      1%      0%
bitmap_scale_filter_90_80      1%      0%
bitmap_scale_filter_90_90      1%      1%
bitmap_scale_filter_80_90      0%      0%
bitmap_scale_filter_30_90      3%      6%
bitmap_scale_filter_10_90      0%      2%

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481733003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed: https://skia.googlesource.com/skia/+/3f321351c9a79f3cc797fa2ef6d69db04415cf5b

[xiangze.zhang, 2016-11-07 09:20:26]

Description was changed from

==========
Make SSE2/Neon convolution functions not to read extra bytes

This change makes SSE2/Neon horizental convolution functions do not read
extra pixels past the end of the buffer. Also removes SIMD specific
codes in SkConvolver.

Performance impact is small. I see SSE2 has < 1% regress in nanobench.

BUG=skia:
==========

to

==========
Make SSE2/Neon convolution functions not to read extra bytes

This change makes SSE2/Neon horizental convolution functions do not read
extra pixels past the end of the buffer. Also removes SIMD specific
codes in SkConvolver.

Performance impact is small. I see SSE2 has < 1% regress in nanobench.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481733003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[xiangze.zhang, 2016-11-09 02:47:19]

Description was changed from

==========
Make SSE2/Neon convolution functions not to read extra bytes

This change makes SSE2/Neon horizental convolution functions do not read
extra pixels past the end of the buffer. Also removes SIMD specific
codes in SkConvolver.

Performance impact is small. I see SSE2 has < 1% regress in nanobench.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481733003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Make SSE2/Neon convolution functions not to read extra bytes

This change makes SSE2/Neon horizontal convolution functions do not read
extra pixels past the end of the buffer. So we can remove all the SIMD 
specific logic in SkConvolver to deal with last couple of rows and also 
avoid applying padding to convolution filters.

Performance impact is small. Nanobench time change: 
                              SSE2    NEON
bitmap_scale_filter_64_256     1%     -2%
bitmap_scale_filter_256_64     1%      2%
bitmap_scale_filter_90_10      1%     -1%
bitmap_scale_filter_90_30      1%      0%
bitmap_scale_filter_90_80      1%      0%
bitmap_scale_filter_90_90      1%      1%
bitmap_scale_filter_80_90      0%      0%
bitmap_scale_filter_30_90      3%      6%
bitmap_scale_filter_10_90      0%      2%

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481733003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[xiangze.zhang, 2016-11-09 02:53:22]

If this patch is ok, next step I'd like to port convolution functions to SkOpts
framework and add AVX2 support.

[xiangze.zhang, 2016-11-09 03:04:57]

xiangze.zhang@intel.com changed reviewers:
+ mtklein@chromium.org

[xiangze.zhang, 2016-11-09 03:13:55]

If this patch is ok, next step I'd like to port convolution functions to SkOpts
framework and add AVX2 support.

[mtklein_C, 2016-11-09 09:14:23]

The CQ bit was checked by mtklein@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 09:14:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 09:22:54]

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

[commit-bot: I haz the power, 2016-11-09 09:22:56]

Dry run: Try jobs failed on following builders:
  Housekeeper-PerCommit-InfraTests on skia.primary (JOB_FAILED,
https://task-scheduler.skia.org/job/20161109T091539.079082292Z_0000000000000ada)

[mtklein_C, 2016-11-09 09:25:04]

I've marked a few questions but only about superficial matters.  The substance
of this looks good to me and further, delights me.  I'm very eager to help you
get this landed.

https://codereview.chromium.org/2481733003/diff/60001/src/opts/SkBitmapFilter...
File src/opts/SkBitmapFilter_opts_SSE2.cpp (right):

https://codereview.chromium.org/2481733003/diff/60001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts_SSE2.cpp:120: #define ACCUM_REMAINDER(src, accum) {
                                                         \
Can we make these ACCUM_REMAINDERs lambdas or static functions instead of
macros?  I don't see anything too sneaky going on here that really requires a
macro.  If needed, SK_ALWAYS_INLINE is pretty reliable at forcing inlining.

(Same question in the NEON code of course.)

https://codereview.chromium.org/2481733003/diff/60001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts_SSE2.cpp:130: __m128i t =
_mm_set_epi32(remainder[3], remainder[2], remainder[1], remainder[0]); \
This is not a big deal, but we have been trending towards using _mm_setr
versions of these functions, mostly to keep things reading consistently when you
switch over to NEON or think about how the vector would be stored in memory.

https://codereview.chromium.org/2481733003/diff/60001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts_SSE2.cpp:131: accum = _mm_add_epi32(accum, t); }
If we do need to use macros, it's probably best to add another \ and put the }
on its own line.  It's sort of easy to overlook here.

[mtklein_C, 2016-11-09 09:26:27]

I think "Housekeeper-PerCommit-InfraTests" is a new test bot, and it must not be
reliable yet.  It's unlikely you could have affected it.  Don't worry if you see
it marked red.

[xiangze.zhang, 2016-11-10 03:16:56]

https://codereview.chromium.org/2481733003/diff/60001/src/opts/SkBitmapFilter...
File src/opts/SkBitmapFilter_opts_SSE2.cpp (right):

https://codereview.chromium.org/2481733003/diff/60001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts_SSE2.cpp:120: #define ACCUM_REMAINDER(src, accum) {
                                                         \
On 2016/11/09 09:25:04, mtklein_C wrote:
> Can we make these ACCUM_REMAINDERs lambdas or static functions instead of
> macros?  I don't see anything too sneaky going on here that really requires a
> macro.  If needed, SK_ALWAYS_INLINE is pretty reliable at forcing inlining.
> 
> (Same question in the NEON code of course.)

Done.

https://codereview.chromium.org/2481733003/diff/60001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts_SSE2.cpp:130: __m128i t =
_mm_set_epi32(remainder[3], remainder[2], remainder[1], remainder[0]); \
On 2016/11/09 09:25:04, mtklein_C wrote:
> This is not a big deal, but we have been trending towards using _mm_setr
> versions of these functions, mostly to keep things reading consistently when
you
> switch over to NEON or think about how the vector would be stored in memory.

Done.

[mtklein_C, 2016-11-10 13:14:36]

The CQ bit was checked by mtklein@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 13:14:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mtklein_C, 2016-11-10 13:20:42]

lgtm!

Thanks for the updates.

[commit-bot: I haz the power, 2016-11-10 13:42:14]

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

[commit-bot: I haz the power, 2016-11-10 13:42:15]

Dry run: This issue passed the CQ dry run.

[mtklein_C, 2016-11-10 13:43:25]

The CQ bit was checked by mtklein@chromium.org

[commit-bot: I haz the power, 2016-11-10 13:43:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 13:44:34]

Description was changed from

==========
Make SSE2/Neon convolution functions not to read extra bytes

This change makes SSE2/Neon horizontal convolution functions do not read
extra pixels past the end of the buffer. So we can remove all the SIMD 
specific logic in SkConvolver to deal with last couple of rows and also 
avoid applying padding to convolution filters.

Performance impact is small. Nanobench time change: 
                              SSE2    NEON
bitmap_scale_filter_64_256     1%     -2%
bitmap_scale_filter_256_64     1%      2%
bitmap_scale_filter_90_10      1%     -1%
bitmap_scale_filter_90_30      1%      0%
bitmap_scale_filter_90_80      1%      0%
bitmap_scale_filter_90_90      1%      1%
bitmap_scale_filter_80_90      0%      0%
bitmap_scale_filter_30_90      3%      6%
bitmap_scale_filter_10_90      0%      2%

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481733003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Make SSE2/Neon convolution functions not to read extra bytes

This change makes SSE2/Neon horizontal convolution functions do not read
extra pixels past the end of the buffer. So we can remove all the SIMD 
specific logic in SkConvolver to deal with last couple of rows and also 
avoid applying padding to convolution filters.

Performance impact is small. Nanobench time change: 
                              SSE2    NEON
bitmap_scale_filter_64_256     1%     -2%
bitmap_scale_filter_256_64     1%      2%
bitmap_scale_filter_90_10      1%     -1%
bitmap_scale_filter_90_30      1%      0%
bitmap_scale_filter_90_80      1%      0%
bitmap_scale_filter_90_90      1%      1%
bitmap_scale_filter_80_90      0%      0%
bitmap_scale_filter_30_90      3%      6%
bitmap_scale_filter_10_90      0%      2%

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2481733003
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed:
https://skia.googlesource.com/skia/+/3f321351c9a79f3cc797fa2ef6d69db04415cf5b
==========

[commit-bot: I haz the power, 2016-11-10 13:44:35]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/3f321351c9a79f3cc797fa2ef6d69db04415cf5b