Port convolve functions to SkOpts

Port convolve functions to SkOpts

This patch moves the C++/SSE2/NEON implementations of convolve functions
into the same place and uses SkOpts framework.
Also some indentation fix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2500113004
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/+/d2265e537c8015f8115d7b5b7f6de970aa688172

[xiangze.zhang, 2016-11-15 07:18:47]

Description was changed from

==========
Port convolve functions to SkOpts framework

Also fix indentation.

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

to

==========
Port convolve functions to SkOpts framework

Also fix indentation.

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

[xiangze.zhang, 2016-11-16 06:49:35]

Description was changed from

==========
Port convolve functions to SkOpts framework

Also fix indentation.

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

to

==========
Port convolve functions to SkOpts

This patch moves the C++/SSE2/NEON implementations of convolve functions
into the same place and uses SkOpts framework.
Also some indentation fix.

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

[xiangze.zhang, 2016-11-16 06:49:36]

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

[xiangze.zhang, 2016-11-16 08:50:18]

PTAL.

I have another question, I think I might as well bring up it here.

My previous patch(https://codereview.chromium.org/2481733003) tries to avoid
horizontal convolution function to read extra bytes. But the vertical
convolution function seems to also read extra bytes. And skia does not take
special care about it and it works fine. So I wonder if it is a necessary to
avoid the 128 bits read near the end of the buffer? Or do I miss something here?

[mtklein_C, 2016-11-16 14:07:31]

> I have another question, I think I might as well bring up it here.
> 
> My previous patch(https://codereview.chromium.org/2481733003) tries to avoid
horizontal convolution function to read extra bytes. But the vertical
convolution function seems to also read extra bytes. And skia does not take
special care about it and it works fine. So I wonder if it is a necessary to
avoid the 128 bits read near the end of the buffer? Or do I miss something here?

I suspect this is a bug in Skia.  If you think you can write up a patch to fix
it, I'd be very happy to review and land it.

[mtklein_C, 2016-11-16 14:24:47]

I have only minor comments.  This looks great.

https://codereview.chromium.org/2500113004/diff/20001/src/opts/SkBitmapFilter...
File src/opts/SkBitmapFilter_opts.h (right):

https://codereview.chromium.org/2500113004/diff/20001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts.h:17: #include <emmintrin.h>
This will #include the SSE intrinsics into the SK_OPTS_NS namespace (sse2, etc).
 This is somewhat unusual.  It probably will work, but it sort of looks like
we're doing it because we _have_ to.

We usually put this at the top before opening the SK_OPTS_NS namespace.

#if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2
    #include <immintrin.h>
#endif

https://codereview.chromium.org/2500113004/diff/20001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts.h:403: #include <arm_neon.h>
Same deal with emmintrin.  It's probably best to put the include outside the
namespace.

https://codereview.chromium.org/2500113004/diff/20001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts.h:912: void (*convolve_4_rows_horizontally)(const
unsigned char* src_data[4],
Do you think it'd make the calling code clearer to just implement this in terms
of convolve_horizontally?  (Maybe that's best left as a follow up.)

[mtklein_C, 2016-11-16 14:25:00]

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

[commit-bot: I haz the power, 2016-11-16 14:25:08]

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-16 14:25:58]

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

[commit-bot: I haz the power, 2016-11-16 14:25:59]

Dry run: Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
  Build-Ubuntu-Clang-arm-Debug-GN_Android-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...)
  Build-Ubuntu-Clang-arm64-Debug-GN_Android-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...)
  Build-Ubuntu-Clang-arm64-Debug-GN_Android_FrameworkDefs-Trybot on
master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...)
  Build-Ubuntu-Clang-mips64el-Debug-GN_Android-Trybot on
master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-m...)
  Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
  Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Release-Vulkan-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[xiangze.zhang, 2016-11-17 02:33:07]

https://codereview.chromium.org/2500113004/diff/20001/src/opts/SkBitmapFilter...
File src/opts/SkBitmapFilter_opts.h (right):

https://codereview.chromium.org/2500113004/diff/20001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts.h:17: #include <emmintrin.h>
On 2016/11/16 14:24:47, mtklein_C wrote:
> This will #include the SSE intrinsics into the SK_OPTS_NS namespace (sse2,
etc).
>  This is somewhat unusual.  It probably will work, but it sort of looks like
> we're doing it because we _have_ to.
> 
> We usually put this at the top before opening the SK_OPTS_NS namespace.
> 
> #if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2
>     #include <immintrin.h>
> #endif

Done.

https://codereview.chromium.org/2500113004/diff/20001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts.h:403: #include <arm_neon.h>
On 2016/11/16 14:24:47, mtklein_C wrote:
> Same deal with emmintrin.  It's probably best to put the include outside the
> namespace.

Done.

https://codereview.chromium.org/2500113004/diff/20001/src/opts/SkBitmapFilter...
src/opts/SkBitmapFilter_opts.h:912: void (*convolve_4_rows_horizontally)(const
unsigned char* src_data[4],
On 2016/11/16 14:24:47, mtklein_C wrote:
> Do you think it'd make the calling code clearer to just implement this in
terms
> of convolve_horizontally?  (Maybe that's best left as a follow up.)

The calling code can check and allocate a smaller row buffer when
convolve_4_rows_horizontally is nullptr. So I'd like to not change it for now.

[xiangze.zhang, 2016-11-17 02:38:51]

On 2016/11/16 14:07:31, mtklein_C wrote:
> > I have another question, I think I might as well bring up it here.
> > 
> > My previous patch(https://codereview.chromium.org/2481733003) tries to avoid
> horizontal convolution function to read extra bytes. But the vertical
> convolution function seems to also read extra bytes. And skia does not take
> special care about it and it works fine. So I wonder if it is a necessary to
> avoid the 128 bits read near the end of the buffer? Or do I miss something
here?
> 
> I suspect this is a bug in Skia.  If you think you can write up a patch to fix
> it, I'd be very happy to review and land it.

Seems I have missed something here. Vertical convolution function reads from a
row buffer and the rows is padded to be aligned to 16 bytes, so it is not an
issue:)

[mtklein_C, 2016-11-17 12:55:32]

On 2016/11/17 at 02:38:51, xiangze.zhang wrote:
> On 2016/11/16 14:07:31, mtklein_C wrote:
> > > I have another question, I think I might as well bring up it here.
> > > 
> > > My previous patch(https://codereview.chromium.org/2481733003) tries to
avoid
> > horizontal convolution function to read extra bytes. But the vertical
> > convolution function seems to also read extra bytes. And skia does not take
> > special care about it and it works fine. So I wonder if it is a necessary to
> > avoid the 128 bits read near the end of the buffer? Or do I miss something
here?
> > 
> > I suspect this is a bug in Skia.  If you think you can write up a patch to
fix
> > it, I'd be very happy to review and land it.
> 
> Seems I have missed something here. Vertical convolution function reads from a
row buffer and the rows is padded to be aligned to 16 bytes, so it is not an
issue:)

Ah, good.  Thanks for looking.

[mtklein_C, 2016-11-17 12:56:04]

The CQ bit was checked by mtklein@chromium.org

[mtklein_C, 2016-11-17 12:56:04]

lgtm

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

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-17 13:00:17]

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

[commit-bot: I haz the power, 2016-11-17 13:00:18]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-mips64el-Debug-GN_Android-Trybot on
master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-m...)

[mtklein_C, 2016-11-17 13:09:21]

On 2016/11/17 at 13:00:18, commit-bot wrote:
> Try jobs failed on following builders:
>   Build-Ubuntu-Clang-mips64el-Debug-GN_Android-Trybot on
master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-m...)

I think this is the bot telling us our non-SSE, non-NEON code path has a typo.

In file included from ../../../src/core/SkOpts.cpp:39:
../../../src/opts/SkBitmapFilter_opts.h:930:13: error: no template named
'convolveVertically'; did you mean 'ConvolveVertically'?
            convolveVertically<true>(filter_values, filter_length,
source_data_rows,
            ^~~~~~~~~~~~~~~~~~
            ConvolveVertically
../../../src/opts/SkBitmapFilter_opts.h:828:10: note: 'ConvolveVertically'
declared here
    void ConvolveVertically(const SkConvolutionFilter1D::ConvolutionFixed*
filterValues,
         ^
../../../src/opts/SkBitmapFilter_opts.h:933:13: error: no template named
'convolveVertically'; did you mean 'ConvolveVertically'?
            convolveVertically<false>(filter_values, filter_length,
source_data_rows,
            ^~~~~~~~~~~~~~~~~~
            ConvolveVertically
../../../src/opts/SkBitmapFilter_opts.h:828:10: note: 'ConvolveVertically'
declared here
    void ConvolveVertically(const SkConvolutionFilter1D::ConvolutionFixed*
filterValues,

[xiangze.zhang, 2016-11-18 02:09:05]

The CQ bit was checked by xiangze.zhang@intel.com

[xiangze.zhang, 2016-11-18 02:09:06]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org
Link to the patchset: https://codereview.chromium.org/2500113004/#ps100001
(title: "Fix typo")

[commit-bot: I haz the power, 2016-11-18 02:09:16]

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-18 02:39:40]

Description was changed from

==========
Port convolve functions to SkOpts

This patch moves the C++/SSE2/NEON implementations of convolve functions
into the same place and uses SkOpts framework.
Also some indentation fix.

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

to

==========
Port convolve functions to SkOpts

This patch moves the C++/SSE2/NEON implementations of convolve functions
into the same place and uses SkOpts framework.
Also some indentation fix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2500113004
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/+/d2265e537c8015f8115d7b5b7f6de970aa688172
==========

[commit-bot: I haz the power, 2016-11-18 02:39:41]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/d2265e537c8015f8115d7b5b7f6de970aa688172