Change in libosmocore[master]: libomsocoding: NEON viterbi acceleration

[pespin]

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/19372 )

Change subject: libomsocoding: NEON viterbi acceleration
......................................................................


Patch Set 1:

(6 comments)

https://gerrit.osmocom.org/c/libosmocore/+/19372/1//COMMIT_MSG 
Commit Message:

https://gerrit.osmocom.org/c/libosmocore/+/19372/1//COMMIT_MSG@9 
PS1, Line 9: configure flag required  to enable this: --enable-neon
double space "required  to"


https://gerrit.osmocom.org/c/libosmocore/+/19372/1//COMMIT_MSG@12 
PS1, Line 12: is only defined if the fpu is neon neon-fp16 neon-vfpv3 eon-vfpv4
type: missing n in nenon.


https://gerrit.osmocom.org/c/libosmocore/+/19372/1//COMMIT_MSG@14 
PS1, Line 14: performance impact, so it needs to be enabled manually.
I don't get what do you mean here. What do you mean with "unknown performance impact" here, why?


https://gerrit.osmocom.org/c/libosmocore/+/19372/1/configure.ac 
File configure.ac:

https://gerrit.osmocom.org/c/libosmocore/+/19372/1/configure.ac@388 
PS1, Line 388: [Support ARM NEON instructions])
I would say this flag is more "Enable neon instruction specific optimizations", because you can still disable it regardless of your target supporting neon or not.


https://gerrit.osmocom.org/c/libosmocore/+/19372/1/src/conv_acc.c 
File src/conv_acc.c:

https://gerrit.osmocom.org/c/libosmocore/+/19372/1/src/conv_acc.c@557 
PS1, Line 557: 			dec->metric_func = osmo_conv_metrics_k5_n2;
wrong indentation.

Probably rather:
#ifdef HAVE_NEON
if (code->len < 100) {
    dec->metric_func = osmo_conv_gen_metrics_k5_n2;
} else
#endif
{
    dec->metric_func = osmo_conv_metrics_k5_n2;
}


https://gerrit.osmocom.org/c/libosmocore/+/19372/1/src/conv_acc_neon_impl.h 
File src/conv_acc_neon_impl.h:

https://gerrit.osmocom.org/c/libosmocore/+/19372/1/src/conv_acc_neon_impl.h@26 
PS1, Line 26: /* Some distributions (notably Alpine Linux) for some strange reason
In my linux (ArchLinux) __always_inline is defined in /usr/include/linux/stddef.h. Perhaps you are missing to include a header defining it?



-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/19372
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I58ff2cb4ce3514f43390ff0a2121f81e6a4983b5
Gerrit-Change-Number: 19372
Gerrit-PatchSet: 1
Gerrit-Owner: Hoernchen <ewild at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Thu, 23 Jul 2020 08:39:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200723/0be673bc/attachment.htm>