opus - Nov 2014 - [RFC PATCHv1] cover: celt_pitch_xcorr: Introduce ARM neon intrinsics

On 25 November 2014 at 10:11, Viswanath Puttagunta
<viswanath.puttagunta at linaro.org> wrote:
>
> On 25 November 2014 at 09:39, Jonathan Lennox <jonathan at vidyo.com>
wrote:
> >
> > On Nov 25, 2014, at 10:07 AM, Viswanath Puttagunta
<viswanath.puttagunta at linaro.org> wrote:
> >>
> >> > Also is there plans to make the NEON optimisations on ARMv7
run time
> >> > detectable like they have in cairo/pixman? For generic
distributions
> >> > it would nice to be able to be able to enable them as they
offer
> >> > decent performance improvements but have the code fall back
on devices
> >> > that don't support NEON.
> >> Yep, adding support for ARMv8 is the final objective. I did not
want to introduce too many changes in the first shot... and hence only
introduced for ARMv7. In theory, most of the code (neon intrinsic code) in this
patch should remain unchanged for ARMv8. Only the mechanism by which neon/asimd
presence is detected during runtime and the flags used during compile are the
only ones that should change. I will work on this once this patch gets reviewed
and accepted. I made sure these changes are fairly localized.
> >>
> >> And yes, this patch also supports runtime detection of neon.
Actually, most of code to do run time detection of neon was already there in the
project before this patch. I just re-used the infrastructure.
> >
> > ARMv8 shouldn?t need Neon detection at all ? Neon is a mandatory part
of the ARMv8 architecture, unlike ARMv7, where it?s optional.
> As I understand, your statement for ARMv8 is true for AAarch64 mode. But
for ARMv8 in AAarch32 mode, neon is still optional (although I haven't heard
of an implementation that does not support NEON even in AAarch32 mode). So, for
AArch64 mode, I think rtcd can be disabled. Also, even the neon detection
procedure currently in opus will not work on ARMv8 AArch32 mode. Please refer
>
http://community.arm.com/groups/android-community/blog/2014/10/10/runtime-detection-of-cpu-features-on-an-armv8-a-cpu
> This is one of my todo lists after this patchset gets reviewed and
accepted.
>
> It looks like this is what the configure script is already doing ? arm64
sets rtcd_support to no.
I can't find any evidence of this in configure.ac. Can you please
point me to where rtcd_support is set to no for armv8 (AAarch64)?
> >
> > I believe iOS, Windows RT/Windows Phone 8, and Blackberry 10 all
require CPU support for Neon when running on ARMv7+ platforms, so detection
shouldn?t be necessary there either.  The configure script should probably
default rtcd_support accordingly, but configuring with --disable-rtcd should be
sufficient to build on these platforms, regardless.
>

On Nov 25, 2014, at 11:13 AM, Viswanath Puttagunta <viswanath.puttagunta at
linaro.org<mailto:viswanath.puttagunta at linaro.org>> wrote:

On 25 November 2014 at 10:11, Viswanath Puttagunta
<viswanath.puttagunta at linaro.org<mailto:viswanath.puttagunta at
linaro.org>> wrote:

On 25 November 2014 at 09:39, Jonathan Lennox <jonathan at
vidyo.com<mailto:jonathan at vidyo.com>> wrote:

On Nov 25, 2014, at 10:07 AM, Viswanath Puttagunta <viswanath.puttagunta at
linaro.org<mailto:viswanath.puttagunta at linaro.org>> wrote:

Also is there plans to make the NEON optimisations on ARMv7 run time
detectable like they have in cairo/pixman? For generic distributions
it would nice to be able to be able to enable them as they offer
decent performance improvements but have the code fall back on devices
that don't support NEON.
Yep, adding support for ARMv8 is the final objective. I did not want to
introduce too many changes in the first shot... and hence only introduced for
ARMv7. In theory, most of the code (neon intrinsic code) in this patch should
remain unchanged for ARMv8. Only the mechanism by which neon/asimd presence is
detected during runtime and the flags used during compile are the only ones that
should change. I will work on this once this patch gets reviewed and accepted. I
made sure these changes are fairly localized.

And yes, this patch also supports runtime detection of neon. Actually, most of
code to do run time detection of neon was already there in the project before
this patch. I just re-used the infrastructure.

ARMv8 shouldn?t need Neon detection at all ? Neon is a mandatory part of the
ARMv8 architecture, unlike ARMv7, where it?s optional.
As I understand, your statement for ARMv8 is true for AAarch64 mode. But for
ARMv8 in AAarch32 mode, neon is still optional (although I haven't heard of
an implementation that does not support NEON even in AAarch32 mode). So, for
AArch64 mode, I think rtcd can be disabled. Also, even the neon detection
procedure currently in opus will not work on ARMv8 AArch32 mode. Please refer
http://community.arm.com/groups/android-community/blog/2014/10/10/runtime-detection-of-cpu-features-on-an-armv8-a-cpu
This is one of my todo lists after this patchset gets reviewed and accepted.

It looks like this is what the configure script is already doing ? arm64 sets
rtcd_support to no.
I can't find any evidence of this in configure.ac. Can you please
point me to where rtcd_support is set to no for armv8 (AAarch64)?

Tip of git, starting line 202:

    202     case $host_cpu in
    203           arm64*|aarch64*)
    204         dnl Currently we only have asm for fixed-point
    205         AS_IF([test "$enable_float" != "yes"],[
    206             cpu_arm64=yes
    207             AC_DEFINE([OPUS_ARM64_ASM], [],  [Make use of ARM64 asm
optimization])
    208             AS_GCC_INLINE_ASSEMBLY(
    209                 [inline_optimization="ARM64"],
    210                 [inline_optimization="disabled"]
    211             )
    212             AS_IF([test x"$inline_optimization" =
x"ARM64"],[
    213                 AM_CONDITIONAL([OPUS_ARM64_INLINE_ASM],[true])
    214                 AC_DEFINE([OPUS_ARM64_INLINE_ASM], 1,
    215                     [Use ARM64 inline asm optimizations])
    216             ])
    217                         dnl Don't yet have external asm for arm64
    218                         asm_optimization="disabled"
    219                         dnl Don't need RTCD for arm64
    220                         rtcd_support=no
    221         ])
    222           ;;

This actually should probably be modified to support compilers that have ARM
intrinsics but not GCC assembly (e.g. Visual Studio), however.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://lists.xiph.org/pipermail/opus/attachments/20141125/41fb7b93/attachment.htm

On 25 November 2014 at 10:18, Jonathan Lennox <jonathan at vidyo.com>
wrote:
>
> On Nov 25, 2014, at 11:13 AM, Viswanath Puttagunta
> <viswanath.puttagunta at linaro.org> wrote:
>
> On 25 November 2014 at 10:11, Viswanath Puttagunta
> <viswanath.puttagunta at linaro.org> wrote:
>
>
> On 25 November 2014 at 09:39, Jonathan Lennox <jonathan at vidyo.com>
wrote:
>
>
> On Nov 25, 2014, at 10:07 AM, Viswanath Puttagunta
> <viswanath.puttagunta at linaro.org> wrote:
>
>
> Also is there plans to make the NEON optimisations on ARMv7 run time
> detectable like they have in cairo/pixman? For generic distributions
> it would nice to be able to be able to enable them as they offer
> decent performance improvements but have the code fall back on devices
> that don't support NEON.
>
> Yep, adding support for ARMv8 is the final objective. I did not want to
> introduce too many changes in the first shot... and hence only introduced
> for ARMv7. In theory, most of the code (neon intrinsic code) in this patch
> should remain unchanged for ARMv8. Only the mechanism by which neon/asimd
> presence is detected during runtime and the flags used during compile are
> the only ones that should change. I will work on this once this patch gets
> reviewed and accepted. I made sure these changes are fairly localized.
>
> And yes, this patch also supports runtime detection of neon. Actually, most
> of code to do run time detection of neon was already there in the project
> before this patch. I just re-used the infrastructure.
>
>
> ARMv8 shouldn?t need Neon detection at all ? Neon is a mandatory part of
the
> ARMv8 architecture, unlike ARMv7, where it?s optional.
>
> As I understand, your statement for ARMv8 is true for AAarch64 mode. But
for
> ARMv8 in AAarch32 mode, neon is still optional (although I haven't
heard of
> an implementation that does not support NEON even in AAarch32 mode). So,
for
> AArch64 mode, I think rtcd can be disabled. Also, even the neon detection
> procedure currently in opus will not work on ARMv8 AArch32 mode. Please
> refer
>
http://community.arm.com/groups/android-community/blog/2014/10/10/runtime-detection-of-cpu-features-on-an-armv8-a-cpu
> This is one of my todo lists after this patchset gets reviewed and
accepted.
>
> It looks like this is what the configure script is already doing ? arm64
> sets rtcd_support to no.
>
> I can't find any evidence of this in configure.ac. Can you please
> point me to where rtcd_support is set to no for armv8 (AAarch64)?
>
>
> Tip of git, starting line 202:
>
>     202     case $host_cpu in
>     203           arm64*|aarch64*)
>     204         dnl Currently we only have asm for fixed-point
>     205         AS_IF([test "$enable_float" != "yes"],[
>     206             cpu_arm64=yes
>     207             AC_DEFINE([OPUS_ARM64_ASM], [],  [Make use of ARM64 asm
> optimization])
>     208             AS_GCC_INLINE_ASSEMBLY(
>     209                 [inline_optimization="ARM64"],
>     210                 [inline_optimization="disabled"]
>     211             )
>     212             AS_IF([test x"$inline_optimization" =
x"ARM64"],[
>     213                 AM_CONDITIONAL([OPUS_ARM64_INLINE_ASM],[true])
>     214                 AC_DEFINE([OPUS_ARM64_INLINE_ASM], 1,
>     215                     [Use ARM64 inline asm optimizations])
>     216             ])
>     217                         dnl Don't yet have external asm for
arm64
>     218                         asm_optimization="disabled"
>     219                         dnl Don't need RTCD for arm64
>     220                         rtcd_support=no
>     221         ])
>     222           ;;
>
> This actually should probably be modified to support compilers that have
ARM
> intrinsics but not GCC assembly (e.g. Visual Studio), however.
Hmm... I don't see this on tip of (git://git.xiph.org/opus.git,
"master" branch, commit:
d10755e95c49f5ec925e70598877c82ad865a189). Am I looking at the wrong place?