ARM Skia NEON patches - 32 - Xfermode: 1-pixel NEON modeprocs

ARM Skia NEON patches - 32 - Xfermode: 1-pixel NEON modeprocs

In some cases, it's easy to provide a NEON version of the 1-pixel modeprocs.
Combined with https://codereview.chromium.org/23724013/ (merged) it allows
up to 35% speed improvement on Xfermodes when aa is non-NULL.

Signed-off-by: Kévin PETIT <kevin.petit@arm.com>;

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=12525

[kevin.petit.not.used.account, 2013-12-04 17:02:42]

This code has no mismatches and combined with
https://codereview.chromium.org/23724013/ (merged) allows up to 35% speed
improvement on Xfermodes when aa is non-NULL.

The initialisation of gProcCoeffs could be controversial: it implies running
sk_cpu_arm_has_neon() before the application linking against skia has entered
its main() function. Any opinions appreciated.

[djsollen, 2013-12-05 14:17:04]

https://codereview.chromium.org/104883004/diff/1/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/104883004/diff/1/src/core/SkXfermode.cpp#newc...
src/core/SkXfermode.cpp:645: { SK_ARM_NEON_WRAP(srcatop_modeproc),  
SkXfermode::kDA_Coeff,      SkXfermode::kISA_Coeff },
this is going to be problematic on chrome as their sandbox does not have access
to whether or not the device has neon so that info is populated as part of the
startup process.  In that case you won't get the neon procs even it the device
has them.

[djsollen, 2013-12-05 15:11:36]

https://codereview.chromium.org/104883004/diff/1/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/104883004/diff/1/src/core/SkXfermode.cpp#newc...
src/core/SkXfermode.cpp:127: #if !SK_ARM_NEON_IS_ALWAYS
I would like to remove this.  I think we could do this similar to how we do
other platform specific optimizations.  I suggest that prior to pulling the proc
from gProcCoeffs we call a platformProc funtion that we define in
SkXfermode_opts_* which returns a ProcCoeff if we have a platform specific one
available. If none is found then we default to the standard lookup.

We only lookup the proc once so having it go through an extra function call
should be fine.

[kevin.petit.not.used.account, 2013-12-05 17:02:46]

https://codereview.chromium.org/104883004/diff/1/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/104883004/diff/1/src/core/SkXfermode.cpp#newc...
src/core/SkXfermode.cpp:127: #if !SK_ARM_NEON_IS_ALWAYS
Done. I've used a factory approach, like suggested. It seems a bit overkill but
I couldn't find a simpler (and clean) way.

https://codereview.chromium.org/104883004/diff/1/src/core/SkXfermode.cpp#newc...
src/core/SkXfermode.cpp:645: { SK_ARM_NEON_WRAP(srcatop_modeproc),  
SkXfermode::kDA_Coeff,      SkXfermode::kISA_Coeff },
I thought that would be problematic in a way or another.

[djsollen, 2013-12-05 17:25:19]

https://codereview.chromium.org/104883004/diff/20001/src/opts/SkXfermode_opts...
File src/opts/SkXfermode_opts_none.cpp (right):

https://codereview.chromium.org/104883004/diff/20001/src/opts/SkXfermode_opts...
src/opts/SkXfermode_opts_none.cpp:8: extern SkXfermodeProc
SkPlatformXfermodeProcFactory_impl(SkXfermode::Mode mode);
you don't want _impl here.

https://codereview.chromium.org/104883004/diff/20001/src/opts/SkXfermode_opts...
src/opts/SkXfermode_opts_none.cpp:15: SkXfermodeProc
SkPlatformXfermodeProcFactory_impl(SkXfermode::Mode mode) {
same here

[kevin.petit.not.used.account, 2013-12-05 17:35:38]

https://codereview.chromium.org/104883004/diff/20001/src/opts/SkXfermode_opts...
File src/opts/SkXfermode_opts_none.cpp (right):

https://codereview.chromium.org/104883004/diff/20001/src/opts/SkXfermode_opts...
src/opts/SkXfermode_opts_none.cpp:8: extern SkXfermodeProc
SkPlatformXfermodeProcFactory_impl(SkXfermode::Mode mode);
On 2013/12/05 17:25:19, djsollen wrote:
> you don't want _impl here.

Done.

https://codereview.chromium.org/104883004/diff/20001/src/opts/SkXfermode_opts...
src/opts/SkXfermode_opts_none.cpp:15: SkXfermodeProc
SkPlatformXfermodeProcFactory_impl(SkXfermode::Mode mode) {
On 2013/12/05 17:25:19, djsollen wrote:
> same here

Done. Thank you.

[reed1, 2013-12-05 18:39:44]

Do we have before/after bench numbers?

[kevin.petit.not.used.account, 2013-12-05 19:04:28]

On 2013/12/05 18:39:44, reed1 wrote:
> Do we have before/after bench numbers?

Yes, from my first message:

>This code has no mismatches and combined with
>https://codereview.chromium.org/23724013/ (merged) allows up to 35% speed
>improvement on Xfermodes when aa is non-NULL.

[reed1, 2013-12-05 21:04:19]

lgtm

[djsollen, 2013-12-05 21:05:11]

lgtm

[reed1, 2013-12-05 21:05:17]

On 2013/12/05 19:04:28, kevin.petit.arm wrote:
> On 2013/12/05 18:39:44, reed1 wrote:
> > Do we have before/after bench numbers?
> 
> Yes, from my first message:

Thanks. That would be good to paste into the CL description.
> 
> >This code has no mismatches and combined with
> >https://codereview.chromium.org/23724013/ (merged) allows up to 35% speed
> >improvement on Xfermodes when aa is non-NULL.

[kevin.petit.not.used.account, 2013-12-06 11:20:26]

On 2013/12/05 21:05:17, reed1 wrote:
> Thanks. That would be good to paste into the CL description.

I'll do it before committing.

[commit-bot: I haz the power, 2013-12-06 11:23:46]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kevin.petit.arm@gmail.com/104883004/4...

[commit-bot: I haz the power, 2013-12-06 11:32:49]

Change committed as 12525