Re: [dpdk-dev] [PATCH v3 1/5] lpm: add sve support for lookup on Arm platform

[David Marchand <david.marchand@redhat.com>]

On Tue, Jan 12, 2021 at 3:57 AM Ruifeng Wang <ruifeng.wang@arm.com> wrote:
> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> index 1afe55cdc..28b57683b 100644
> --- a/lib/librte_lpm/rte_lpm.h
> +++ b/lib/librte_lpm/rte_lpm.h
> @@ -402,7 +402,11 @@ rte_lpm_lookupx4(const struct rte_lpm *lpm, xmm_t ip, uint32_t hop[4],
>         uint32_t defv);
>
>  #if defined(RTE_ARCH_ARM)
> +#ifdef __ARM_FEATURE_SVE
> +#include "rte_lpm_sve.h"
> +#else
>  #include "rte_lpm_neon.h"
> +#endif
>  #elif defined(RTE_ARCH_PPC_64)
>  #include "rte_lpm_altivec.h"
>  #else
> diff --git a/lib/librte_lpm/rte_lpm_sve.h b/lib/librte_lpm/rte_lpm_sve.h
> new file mode 100644
> index 000000000..2e319373e
> --- /dev/null
> +++ b/lib/librte_lpm/rte_lpm_sve.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Arm Limited
> + */
> +
> +#ifndef _RTE_LPM_SVE_H_
> +#define _RTE_LPM_SVE_H_
> +
> +#include <rte_vect.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +__rte_internal
> +static void

I was looking into use of the __rte_internal tag in the tree.

This helper is called from a inlined API used by applications, so out
of the DPDK build.
It looks like the compiler is not complaining when compiling examples
(I hacked my env to cross compile with gcc 10 + SVE enabled) but this
seems incorrect to me.

Is there really a need for this helper?
It is only used below afaics.


> +__rte_lpm_lookup_vec(const struct rte_lpm *lpm, const uint32_t *ips,
> +               uint32_t *__rte_restrict next_hops, const uint32_t n)
> +{

[snip]


> +}
> +
> +static inline void
> +rte_lpm_lookupx4(const struct rte_lpm *lpm, xmm_t ip, uint32_t hop[4],
> +               uint32_t defv)
> +{
> +       uint32_t i, ips[4];
> +
> +       vst1q_s32((int32_t *)ips, ip);
> +       for (i = 0; i < 4; i++)
> +               hop[i] = defv;
> +
> +       __rte_lpm_lookup_vec(lpm, ips, hop, 4);
> +}


--
David Marchand