enable SSE2 in skia/convolver for linux32

enable SSE2 in skia/convolver for linux32
BUG=181072
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192965

[Alpha Left Google, 2013-03-30 04:16:24]

There's also merge problems with convolver_SSE2.cc.

https://codereview.chromium.org/13293004/diff/5001/skia/ext/convolver.cc
File skia/ext/convolver.cc (right):

https://codereview.chromium.org/13293004/diff/5001/skia/ext/convolver.cc#newc...
skia/ext/convolver.cc:299: void ConvolveHorizontally4_SSE2(const unsigned char*
src_data[4],
nit: More meaningful description than just 4?

[hubbe, 2013-04-04 19:30:50]

Adding senorblanco for OWNERS.

https://codereview.chromium.org/13293004/diff/5001/skia/ext/convolver.cc
File skia/ext/convolver.cc (right):

https://codereview.chromium.org/13293004/diff/5001/skia/ext/convolver.cc#newc...
skia/ext/convolver.cc:299: void ConvolveHorizontally4_SSE2(const unsigned char*
src_data[4],
On 2013/03/30 04:16:24, Alpha wrote:
> nit: More meaningful description than just 4?

Done.

[Stephen White, 2013-04-04 20:19:43]

https://codereview.chromium.org/13293004/diff/11001/skia/ext/convolver.cc
File skia/ext/convolver.cc (right):

https://codereview.chromium.org/13293004/diff/11001/skia/ext/convolver.cc#new...
skia/ext/convolver.cc:335: extern void ConvolveHorizontally_SSE2(
These externs are really gross.  Is there a reason you can't put these functions
in a .h?

[hubbe, 2013-04-05 08:18:05]

https://codereview.chromium.org/13293004/diff/11001/skia/ext/convolver.cc
File skia/ext/convolver.cc (right):

https://codereview.chromium.org/13293004/diff/11001/skia/ext/convolver.cc#new...
skia/ext/convolver.cc:335: extern void ConvolveHorizontally_SSE2(
On 2013/04/04 20:19:44, Stephen White wrote:
> These externs are really gross.  Is there a reason you can't put these
functions
> in a .h?

Done.
I agree actually. When I first wrote it I thought the locality would be kind of
nice, but it's kind of weird, isn't it?

[Stephen White, 2013-04-05 08:40:47]

LGTM.

The non-SSE2 wrapper could also be left for a future patch; I leave it up to
you.

https://codereview.chromium.org/13293004/diff/25001/skia/ext/convolver_SSE2.cc
File skia/ext/convolver_SSE2.cc (right):

https://codereview.chromium.org/13293004/diff/25001/skia/ext/convolver_SSE2.c...
skia/ext/convolver_SSE2.cc:435: void ConvolveVertically_SSE2(const
ConvolutionFilter1D::Fixed* filter_values,
It might be a good idea to add a similar wrapper for the non-SSE2 flavour as
well.  It would clean up the call site a bit to hide the template parameters in
the same way, as well as making it more symmetrical with the SSE2 path.

https://codereview.chromium.org/13293004/diff/25001/skia/ext/convolver_unitte...
File skia/ext/convolver_unittest.cc (left):

https://codereview.chromium.org/13293004/diff/25001/skia/ext/convolver_unitte...
skia/ext/convolver_unittest.cc:216: if (!cpu.has_sse2()) return;
Just for my own edification, we're no longer doing the runtime check here
because it's done inside BGRAConvolve2D now, is that correct?  So presumably, on
a non-SSE2 CPU we'd just do two passes of the C code in this test?  (Not a big
deal, just trying to understand.)

[hubbe, 2013-04-06 20:45:45]

https://codereview.chromium.org/13293004/diff/25001/skia/ext/convolver_SSE2.cc
File skia/ext/convolver_SSE2.cc (right):

https://codereview.chromium.org/13293004/diff/25001/skia/ext/convolver_SSE2.c...
skia/ext/convolver_SSE2.cc:435: void ConvolveVertically_SSE2(const
ConvolutionFilter1D::Fixed* filter_values,
On 2013/04/05 08:40:47, Stephen White wrote:
> It might be a good idea to add a similar wrapper for the non-SSE2 flavour as
> well.  It would clean up the call site a bit to hide the template parameters
in
> the same way, as well as making it more symmetrical with the SSE2 path.

Done.

https://codereview.chromium.org/13293004/diff/25001/skia/ext/convolver_unitte...
File skia/ext/convolver_unittest.cc (left):

https://codereview.chromium.org/13293004/diff/25001/skia/ext/convolver_unitte...
skia/ext/convolver_unittest.cc:216: if (!cpu.has_sse2()) return;
On 2013/04/05 08:40:47, Stephen White wrote:
> Just for my own edification, we're no longer doing the runtime check here
> because it's done inside BGRAConvolve2D now, is that correct?  So presumably,
on
> a non-SSE2 CPU we'd just do two passes of the C code in this test?  (Not a big
> deal, just trying to understand.)

That is correct. I also foresee a future where there may be other kinds of
vectorization for this code, and the caller shouldn't have to know about that. I
left the "use_simd" option in there to simplify testing though.

[Stephen White, 2013-04-06 22:44:33]

LGTM too.

https://codereview.chromium.org/13293004/diff/32001/skia/ext/convolver.cc
File skia/ext/convolver.cc (right):

https://codereview.chromium.org/13293004/diff/32001/skia/ext/convolver.cc#new...
skia/ext/convolver.cc:440: if (source_has_alpha) {
Not new to this patch, but it seems a little strange that source_has_alpha is
relevant to the C++ path, but not the SSE2 path for the horizontal convolution. 
Maybe I'm missing something.

[hubbe, 2013-04-08 18:32:33]

On 2013/04/06 22:44:33, Stephen White wrote:
> LGTM too.
> 
> https://codereview.chromium.org/13293004/diff/32001/skia/ext/convolver.cc
> File skia/ext/convolver.cc (right):
> 
>
https://codereview.chromium.org/13293004/diff/32001/skia/ext/convolver.cc#new...
> skia/ext/convolver.cc:440: if (source_has_alpha) {
> Not new to this patch, but it seems a little strange that source_has_alpha is
> relevant to the C++ path, but not the SSE2 path for the horizontal
convolution. 
> Maybe I'm missing something.

I don't think there is any speedup to be had from ignoring the alpha in the SSE2
solution. Multiplying 4 values is easier than multiplying 3.

[commit-bot: I haz the power, 2013-04-08 18:33:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/13293004/32001

[commit-bot: I haz the power, 2013-04-08 18:47:50]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 21:01:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/13293004/32001

[commit-bot: I haz the power, 2013-04-09 00:32:03]

Change committed as 192965