Implement a NEON version of morphology. This is good for ~2.2X speedup on Tegra3.

Implement a NEON version of morphology. This is good for ~2.2X speedup on Tegra3.

R=mtklein@google.com, mtklein, reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12219

[Stephen White, 2013-11-10 22:05:34]

Mike &| Mike: PTAL.

It doesn't look like we do runtime detection of NEON as we do for SSE2 (I think
we ship different binaries in Chrome), but someone more Android-savvy should
probably take a look to see if this is the correct approach.

[Tom Hudson, 2013-11-11 12:38:35]

On 2013/11/10 22:05:34, Stephen White wrote:
> Mike &| Mike: PTAL.
> 
> It doesn't look like we do runtime detection of NEON as we do for SSE2 (I
think
> we ship different binaries in Chrome), but someone more Android-savvy should
> probably take a look to see if this is the correct approach.

We certainly *should* be doing runtime detection; Chrome on Android only ships a
single binary.

[djsollen, 2013-11-11 13:12:23]

We do have runtime neon detection and chrome definitely ships with it.  See
SkUtilsArm.h and the other arm files in the opts dir for how it is used.  If you
have any questions feel free to ping me.

FYI...The only ARM device that we test on that doesn't have neon is the xoom.
Also when running the android make script if you pass "-d arm_v7_thumb" that
will give you the same NEON options that chromium builds with.

[Stephen White, 2013-11-11 15:50:19]

On 2013/11/11 13:12:23, djsollen wrote:
> We do have runtime neon detection and chrome definitely ships with it.  See
> SkUtilsArm.h and the other arm files in the opts dir for how it is used.  If
you
> have any questions feel free to ping me.
> 
> FYI...The only ARM device that we test on that doesn't have neon is the xoom.
> Also when running the android make script if you pass "-d arm_v7_thumb" that
> will give you the same NEON options that chromium builds with.

Thanks; fixed and uploaded as patch set 2 (tested on tested on Xoom and Nexus
10).

[mtklein, 2013-11-11 16:05:55]

https://codereview.chromium.org/68123003/diff/50001/src/opts/opts_check_arm.cpp
File src/opts/opts_check_arm.cpp (right):

https://codereview.chromium.org/68123003/diff/50001/src/opts/opts_check_arm.c...
src/opts/opts_check_arm.cpp:71: SkMorphologyProc
SkMorphologyGetPlatformProc(SkMorphologyProcType type) {
I think for completeness' sake, this wants to be:
#if SK_ARM_NEON_IS_NONE
    return NULL;
#elif SK_ARM_NEON_IS_DYNAMIC
    ...
#endif

(Though maybe we should consider dropping SK_ARM_NEON_IS_NONE.  I like thinking
about your code better.)

[Stephen White, 2013-11-11 16:17:12]

On 2013/11/11 16:05:55, mtklein wrote:
>
https://codereview.chromium.org/68123003/diff/50001/src/opts/opts_check_arm.cpp
> File src/opts/opts_check_arm.cpp (right):
> 
>
https://codereview.chromium.org/68123003/diff/50001/src/opts/opts_check_arm.c...
> src/opts/opts_check_arm.cpp:71: SkMorphologyProc
> SkMorphologyGetPlatformProc(SkMorphologyProcType type) {
> I think for completeness' sake, this wants to be:
> #if SK_ARM_NEON_IS_NONE
>     return NULL;
> #elif SK_ARM_NEON_IS_DYNAMIC
>     ...
> #endif
> 
> (Though maybe we should consider dropping SK_ARM_NEON_IS_NONE.  I like
thinking
> about your code better.)

Fixed.. although I couldn't use #elif, since I need the last clause to be true
for DYNAMIC or ALWAYS, but non NONE.

[mtklein, 2013-11-11 16:28:52]

On 2013/11/11 16:17:12, Stephen White wrote:
> On 2013/11/11 16:05:55, mtklein wrote:
> >
>
https://codereview.chromium.org/68123003/diff/50001/src/opts/opts_check_arm.cpp
> > File src/opts/opts_check_arm.cpp (right):
> > 
> >
>
https://codereview.chromium.org/68123003/diff/50001/src/opts/opts_check_arm.c...
> > src/opts/opts_check_arm.cpp:71: SkMorphologyProc
> > SkMorphologyGetPlatformProc(SkMorphologyProcType type) {
> > I think for completeness' sake, this wants to be:
> > #if SK_ARM_NEON_IS_NONE
> >     return NULL;
> > #elif SK_ARM_NEON_IS_DYNAMIC
> >     ...
> > #endif
> > 
> > (Though maybe we should consider dropping SK_ARM_NEON_IS_NONE.  I like
> thinking
> > about your code better.)
> 
> Fixed.. although I couldn't use #elif, since I need the last clause to be true
> for DYNAMIC or ALWAYS, but non NONE.

Ah, because in that case the NEON code's not linked in?

lgtm

[Stephen White, 2013-11-11 16:32:19]

On 2013/11/11 16:28:52, mtklein wrote:
> On 2013/11/11 16:17:12, Stephen White wrote:
> > On 2013/11/11 16:05:55, mtklein wrote:
> > >
> >
>
https://codereview.chromium.org/68123003/diff/50001/src/opts/opts_check_arm.cpp
> > > File src/opts/opts_check_arm.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/68123003/diff/50001/src/opts/opts_check_arm.c...
> > > src/opts/opts_check_arm.cpp:71: SkMorphologyProc
> > > SkMorphologyGetPlatformProc(SkMorphologyProcType type) {
> > > I think for completeness' sake, this wants to be:
> > > #if SK_ARM_NEON_IS_NONE
> > >     return NULL;
> > > #elif SK_ARM_NEON_IS_DYNAMIC
> > >     ...
> > > #endif
> > > 
> > > (Though maybe we should consider dropping SK_ARM_NEON_IS_NONE.  I like
> > thinking
> > > about your code better.)
> > 
> > Fixed.. although I couldn't use #elif, since I need the last clause to be
true
> > for DYNAMIC or ALWAYS, but non NONE.
> 
> Ah, because in that case the NEON code's not linked in?

Even if it is, I have a feeling I'll get an unreachable warning if there's stuff
after an unconditional "return NULL;".

[Stephen White, 2013-11-11 16:49:02]

Committed patchset #3 manually as r12219 (presubmit successful).