RE: [PATCH v2 1/1] mldev: split bfloat16 routines to separate files

[Srikanth Yalavarthi <syalavarthi@marvell.com>]

> -----Original Message-----
> From: Ruifeng Wang <Ruifeng.Wang@arm.com>
> Sent: 15 March 2023 15:32
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Cc: dev@dpdk.org; Shivah Shankar Shankar Narayan Rao
> <sshankarnara@marvell.com>; david.marchand@redhat.com; nd
> <nd@arm.com>
> Subject: [EXT] RE: [PATCH v2 1/1] mldev: split bfloat16 routines to separate
> files
> 
> External Email
> 
> ----------------------------------------------------------------------
> > -----Original Message-----
> > From: Srikanth Yalavarthi <syalavarthi@marvell.com>
> > Sent: Monday, March 13, 2023 8:03 PM
> > To: Srikanth Yalavarthi <syalavarthi@marvell.com>; Ruifeng Wang
> > <Ruifeng.Wang@arm.com>
> > Cc: dev@dpdk.org; sshankarnara@marvell.com;
> david.marchand@redhat.com
> > Subject: [PATCH v2 1/1] mldev: split bfloat16 routines to separate
> > files
> >
> > Since bfloat16 intrinsics are not supported on all ARM platforms that
> > support NEON,
> > bfloat16 routines are moved to separate files.
> > This would enable using scalar implementation for bfloat16 on unsupported
> ARM platforms.
> >
> > Bugzilla ID: 1179
> > Fixes: fc54766b1612 ("mldev: add Arm NEON type conversion")
> >
> > Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> > ---
> > Depends-on: patch-120653 ("mldev: remove weak symbols use in type
> > conversions")
> > Depends-on: patch-125035 ("mldev: fix identical code in conditional
> > branches")
> >
> >  lib/mldev/meson.build                   |  11 +-
> >  lib/mldev/mldev_utils_neon.c            | 142 +------------
> >  lib/mldev/mldev_utils_neon_bfloat16.c   | 154 ++++++++++++++
> >  lib/mldev/mldev_utils_scalar.c          | 262 +-----------------------
> >  lib/mldev/mldev_utils_scalar.h          |  80 ++++++++
> >  lib/mldev/mldev_utils_scalar_bfloat16.c | 197 ++++++++++++++++++
> >  6 files changed, 445 insertions(+), 401 deletions(-)  create mode
> > 100644 lib/mldev/mldev_utils_neon_bfloat16.c
> >  create mode 100644 lib/mldev/mldev_utils_scalar.h  create mode 100644
> > lib/mldev/mldev_utils_scalar_bfloat16.c
> >
> > diff --git a/lib/mldev/meson.build b/lib/mldev/meson.build index
> > c9db42257b..5769b0640a
> > 100644
> > --- a/lib/mldev/meson.build
> > +++ b/lib/mldev/meson.build
> > @@ -7,12 +7,21 @@ sources = files(
> >          'mldev_utils.c',
> >  )
> >
> > -if dpdk_conf.has('RTE_ARCH_ARM64')
> > +if (dpdk_conf.has('RTE_ARCH_ARM64') and
> > +    cc.get_define('__ARM_NEON', args: machine_args) != '')
> 
> I found in ACLE document that "__ARM_NEON" is always set to 1 for
> AArch64".
> So this line of check is redundant?

Checking for __ARM_NEON should be enough.
We can drop the  dpdk_conf.has('RTE_ARCH_ARM64') check.
I will test the builds and submit a revised patch.

> 
> >      sources += files('mldev_utils_neon.c')  else
> >      sources += files('mldev_utils_scalar.c')  endif
> >
> > +if (dpdk_conf.has('RTE_ARCH_ARM64') and
> > +    cc.get_define('__ARM_NEON', args: machine_args) != '' and
> 
> Same here.
> 
> > +    cc.get_define('__ARM_FEATURE_BF16', args: machine_args) != '')
> > +    sources += files('mldev_utils_neon_bfloat16.c')
> > +else
> > +    sources += files('mldev_utils_scalar_bfloat16.c')
> > +endif
> > +
> >  headers = files(
> >          'rte_mldev.h',
> >  )
> <snip>