(Patch by Teodora Novkovic <teodora.petrovic@gmail.com>, originally reviewed at https://codereview…

(Patch by Teodora Novkovic <teodora.petrovic@gmail.com>;, originally reviewed at  https://codereview.chromium.org/14929006/).  

Added MIPS DSPr2 optimization for BGRAConvolve2D routine. 

The following routines are optimized: 
- ConvolveVertically 
- ConvolveHorizontally 

Performance gain measured on Malta 74Kc board: 
- our standalone test/bench application ~45% 
- chromium unit_test ~20% 

R=hubbe@chromium.org, senorblanco@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202197

[Stephen White, 2013-05-22 16:21:54]

hubbe@, would you mind taking a look at this one? It's just a copy of the
original submission with style fixes and AUTHOR changes. Thanks!

[hubbe, 2013-05-22 22:51:27]

lgtm

[commit-bot: I haz the power, 2013-05-23 00:42:42]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Justin Novosad, 2013-05-23 15:32:02]

https://codereview.chromium.org/15742005/diff/1/skia/ext/convolver.h
File skia/ext/convolver.h (right):

https://codereview.chromium.org/15742005/diff/1/skia/ext/convolver.h#newcode23
skia/ext/convolver.h:23: #if defined(ARCH_CPU_MIPS_FAMILY) &&
defined(ARCH_CPU_MIPSEL) && \
Can we simplify this expression? Isn't ARCH_CPU_MIPS_FAMILY implied by
ARCH_CPU_MIPSEL?

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

https://codereview.chromium.org/15742005/diff/1/skia/ext/convolver_SSE2.cc#ne...
skia/ext/convolver_SSE2.cc:20: bool has_alpha) {
Comment-out unused argument to avoid a compiler warning: bool /*has_alpha*/
Log a bug to investigate whether has_alpha could be useful for optimizing this
SSE2 implementation.

https://codereview.chromium.org/15742005/diff/1/skia/ext/convolver_mips_dspr2.cc
File skia/ext/convolver_mips_dspr2.cc (right):

https://codereview.chromium.org/15742005/diff/1/skia/ext/convolver_mips_dspr2...
skia/ext/convolver_mips_dspr2.cc:20: if (has_alpha) {
Since this branch is outside of the loop, we're not getting any performance
benefit from having has_alpha as a template argument.  There is going to be a
run-time conditional branch on has_alpha for each call to this method. The only
difference is whether the branch happens here or in the caller.  Unless there is
tangible evidence (benchmarks?) that prove this has a measurable impact on
performance, this code should all go into the non-template overload below.  On
the other hand, if I am wrong and there is a case for this template, this
function should be in the unnamed namespace (or static).

https://codereview.chromium.org/15742005/diff/1/skia/ext/convolver_mips_dspr2...
skia/ext/convolver_mips_dspr2.cc:246: if (has_alpha) {
Same here.

https://codereview.chromium.org/15742005/diff/1/skia/ext/convolver_mips_dspr2...
skia/ext/convolver_mips_dspr2.cc:485: if (has_alpha) {
This is the conditional branch I was talking about in previous comment. It
executes once per call, so there should be no win from templating, only extra
complexity in the code.

https://codereview.chromium.org/15742005/diff/1/skia/skia.gyp
File skia/skia.gyp (right):

https://codereview.chromium.org/15742005/diff/1/skia/skia.gyp#newcode791
skia/skia.gyp:791: [ 'target_arch != "arm" and target_arch !="mipsel"', {
Missing space

[hubbe, 2013-05-23 17:52:05]

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

https://codereview.chromium.org/15742005/diff/1/skia/ext/convolver_SSE2.cc#ne...
skia/ext/convolver_SSE2.cc:20: bool has_alpha) {
On 2013/05/23 15:32:03, junov wrote:
> Comment-out unused argument to avoid a compiler warning: bool /*has_alpha*/
> Log a bug to investigate whether has_alpha could be useful for optimizing this
> SSE2 implementation.

FYI:
Using has_alpha in the SSE2 implementation has been thought about and rejected
twice already. A bug might be useful to document that though...

[Teodora Novkovic, 2013-05-24 16:02:20]

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/ext/convolver.h
File skia/ext/convolver.h (right):

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/ext/convolver.h#n...
skia/ext/convolver.h:23: #if defined(ARCH_CPU_MIPS_FAMILY) &&
defined(ARCH_CPU_MIPSEL) && \
On 2013/05/23 15:32:03, junov wrote:
> Can we simplify this expression? Isn't ARCH_CPU_MIPS_FAMILY implied by
> ARCH_CPU_MIPSEL?

Done.

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/ext/convolver_SSE...
File skia/ext/convolver_SSE2.cc (right):

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/ext/convolver_SSE...
skia/ext/convolver_SSE2.cc:20: bool has_alpha) {
On 2013/05/23 17:52:05, hubbe wrote:
> On 2013/05/23 15:32:03, junov wrote:
> > Comment-out unused argument to avoid a compiler warning: bool /*has_alpha*/
> > Log a bug to investigate whether has_alpha could be useful for optimizing
this
> > SSE2 implementation.
> 
> FYI:
> Using has_alpha in the SSE2 implementation has been thought about and rejected
> twice already. A bug might be useful to document that though...
> 
Done.

I have logged a bug on:
https://code.google.com/p/chromium/issues/detail?id=243751&thanks=243751&ts=1...

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/ext/convolver_mip...
File skia/ext/convolver_mips_dspr2.cc (right):

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/ext/convolver_mip...
skia/ext/convolver_mips_dspr2.cc:20: if (has_alpha) {
On 2013/05/23 15:32:03, junov wrote:
> Since this branch is outside of the loop, we're not getting any performance
> benefit from having has_alpha as a template argument.  There is going to be a
> run-time conditional branch on has_alpha for each call to this method. The
only
> difference is whether the branch happens here or in the caller.  Unless there
is
> tangible evidence (benchmarks?) that prove this has a measurable impact on
> performance, this code should all go into the non-template overload below.  On
> the other hand, if I am wrong and there is a case for this template, this
> function should be in the unnamed namespace (or static).

Done.

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/ext/convolver_mip...
skia/ext/convolver_mips_dspr2.cc:246: if (has_alpha) {
On 2013/05/23 15:32:03, junov wrote:
> Same here.

Done.

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/ext/convolver_mip...
skia/ext/convolver_mips_dspr2.cc:485: if (has_alpha) {
On 2013/05/23 15:32:03, junov wrote:
> This is the conditional branch I was talking about in previous comment. It
> executes once per call, so there should be no win from templating, only extra
> complexity in the code.

Done.

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/skia.gyp
File skia/skia.gyp (right):

https://chromiumcodereview.appspot.com/15742005/diff/1/skia/skia.gyp#newcode791
skia/skia.gyp:791: [ 'target_arch != "arm" and target_arch !="mipsel"', {
On 2013/05/23 15:32:03, junov wrote:
> Missing space

Done.

[Justin Novosad, 2013-05-24 16:21:11]

lgtm

[commit-bot: I haz the power, 2013-05-24 16:21:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/senorblanco@chromium.org/15742005/25001

[commit-bot: I haz the power, 2013-05-24 22:20:50]

Change committed as 202197