RE: [EXT] Re: [PATCH 1/2] config/arm: avoid mcpu and march conflicts

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

> On Sun, Jan 21, 2024 at 10:37 AM <pbhagavatula@marvell.com> wrote:
> >
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >
> > The compiler options march and mtune are a subset
> > of mcpu and will lead to conflicts if improper march
> > is chosen for a given mcpu.
> > To avoid conflicts, force part number march when
> > mcpu is available and is supported by the compiler.
> >
> > Example:
> >         march = armv9-a
> >         mcpu = neoverse-n2
> >
> >         mcpu supported, march supported
> >         machine_args = ['-mcpu=neoverse-n2', '-march=armv9-a']
> >
> >         mcpu supported, march not supported
> >         machine_args = ['-mcpu=neoverse-n2']
> >
> >         mcpu not supported, march supported
> >         machine_args = ['-march=armv9-a']
> >
> >         mcpu not supported, march not supported
> >         machine_args = ['-march=armv8.6-a']
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > ---
> >  config/arm/meson.build | 109 +++++++++++++++++++++++++--------------
> --
> >  1 file changed, 67 insertions(+), 42 deletions(-)
> >
> > diff --git a/config/arm/meson.build b/config/arm/meson.build
> > index 36f21d2259..8c8cfccca0 100644
> > --- a/config/arm/meson.build
> > +++ b/config/arm/meson.build
> <snip>
> > @@ -127,21 +128,22 @@ implementer_cavium = {
> >      ],
> >      'part_number_config': {
> >          '0xa1': {
> > -            'compiler_options': ['-mcpu=thunderxt88'],
> > +            'mcpu': 'thunderxt88',
> >              'flags': flags_part_number_thunderx
> >          },
> >          '0xa2': {
> > -            'compiler_options': ['-mcpu=thunderxt81'],
> > +            'mcpu': 'thunderxt81',
> >              'flags': flags_part_number_thunderx
> >          },
> >          '0xa3': {
> > -            'compiler_options': ['-march=armv8-a+crc', '-mcpu=thunderxt83'],
> > +            'mcpu': 'thunderxt83',
> > +            'compiler_options': ['-march=armv8-a+crc'],
> 
> Let's unify this with the rest and specify 'march': 'armv8-a+crc'
> instead of having it under compiler_options.

Ack.

> 
> >              'flags': flags_part_number_thunderx
> >          },
> >          '0xaf': {
> >              'march': 'armv8.1-a',
> >              'march_features': ['crc', 'crypto'],
> > -            'compiler_options': ['-mcpu=thunderx2t99'],
> > +            'mcpu': 'thunderx2t99',
> >              'flags': [
> >                  ['RTE_MACHINE', '"thunderx2"'],
> >                  ['RTE_ARM_FEATURE_ATOMICS', true],
> > @@ -153,7 +155,7 @@ implementer_cavium = {
> >          '0xb2': {
> >              'march': 'armv8.2-a',
> >              'march_features': ['crc', 'crypto', 'lse'],
> > -            'compiler_options': ['-mcpu=octeontx2'],
> > +            'mcpu': 'octeontx2',
> >              'flags': [
> >                  ['RTE_MACHINE', '"cn9k"'],
> >                  ['RTE_ARM_FEATURE_ATOMICS', true],
> > @@ -176,7 +178,7 @@ implementer_ampere = {
> >          '0x0': {
> >              'march': 'armv8-a',
> >              'march_features': ['crc', 'crypto'],
> > -            'compiler_options':  ['-mtune=emag'],
> > +            'mcpu': 'emag',
> 
> We're changing mtune to mcpu, is this equivalent?
> 

Both march and mtune are a subset of mcpu.

> >              'flags': [
> >                  ['RTE_MACHINE', '"eMAG"'],
> >                  ['RTE_MAX_LCORE', 32],
> > @@ -186,7 +188,7 @@ implementer_ampere = {
> >          '0xac3': {
> >              'march': 'armv8.6-a',
> >              'march_features': ['crc', 'crypto'],
> > -            'compiler_options':  ['-mcpu=ampere1'],
> > +            'mcpu': 'ampere1',
> >              'flags': [
> >                  ['RTE_MACHINE', '"AmpereOne"'],
> >                  ['RTE_MAX_LCORE', 320],
> > @@ -206,7 +208,7 @@ implementer_hisilicon = {
> >          '0xd01': {
> >              'march': 'armv8.2-a',
> >              'march_features': ['crypto'],
> > -            'compiler_options': ['-mtune=tsv110'],
> > +            'mcpu': 'tsv110',
> >              'flags': [
> >                  ['RTE_MACHINE', '"Kunpeng 920"'],
> >                  ['RTE_ARM_FEATURE_ATOMICS', true],
> > @@ -695,11 +697,23 @@ if update_flags
> >
> >      machine_args = [] # Clear previous machine args
> >
> > +    candidate_mcpu = ''
> > +    support_mcpu = false
> > +    if part_number_config.has_key('mcpu')
> > +        mcpu = part_number_config['mcpu']
> > +        if (cc.has_argument('-mcpu=' + mcpu))
> > +            candidate_mcpu = mcpu
> > +            support_mcpu = true
> > +        endif
> > +    endif
> > +
> >      # probe supported archs and their features
> >      candidate_march = ''
> >      if part_number_config.has_key('march')
> > -        if part_number_config.get('force_march', false)
> > -            candidate_march = part_number_config['march']
> > +        if part_number_config.get('force_march', false) or support_mcpu
> 
> Instead of using the extra "support_mcpu" variable, we could do the
> same check as with candidate march (if candidate_mcpu != '', which we
> actually do below in the last lines of the patch).
> 

Ack.

> If I understand the logic correctly, we don't want to do the march
> fallback if mcpu is specified - either the march works with the given
> mcpu or we do without it (because we don't actually need it with
> mcpu). Is that correct?
> 

Yes, but still exact march defined in part_number_config should be present for setting extra_march_features.
specially for expressing crypto support. 

> > +            if cc.has_argument('-march=' +  part_number_config['march'])
> 
> Now that we've added mcpu into the mix, is this still the right
> condition? Can the below happen?
> 
> This check finds that machine_args = ['-march=armv9-a'] is supported.
> 
> But taken together with mcpu (machine_args = ['-mcpu=neoverse-n2',
> '-march=armv9-a']), it is not supported? In this case we'll end up
> with invalid configuration.

This is the only correct option and evolves into -march=armv9-a+sve2+crypto for cn10k
whereas other neoverse-n2 might only have -march=armv9-a+sve2.

Example:

Good:
#aarch64-linux-gnu-gcc  -march=armv9-a+sve2+crypto -mcpu=neoverse-n2 shrn.c
#aarch64-linux-gnu-gcc  -march=armv9-a+sve2 -mcpu=neoverse-n2 shrn.c 
#aarch64-linux-gnu-gcc  -march=armv9-a -mcpu=neoverse-n2 shrn.c

Bad:
#aarch64-linux-gnu-gcc -mcpu=neoverse-n2 -march=armv8-a shrn.c 
cc1: warning: switch '-mcpu=neoverse-n2' conflicts with '-march=armv8-a' switch
#aarch64-linux-gnu-gcc -mcpu=neoverse-n2 -march=armv8.1-a shrn.c
cc1: warning: switch '-mcpu=neoverse-n2' conflicts with '-march=armv8.1-a' switch
#aarch64-linux-gnu-gcc -mcpu=neoverse-n2 -march=armv8.2-a shrn.c
cc1: warning: switch '-mcpu=neoverse-n2' conflicts with '-march=armv8.2-a' switch
#aarch64-linux-gnu-gcc -mcpu=neoverse-n2 -march=armv8.3-a shrn.c
cc1: warning: switch '-mcpu=neoverse-n2' conflicts with '-march=armv8.3-a' switch
#aarch64-linux-gnu-gcc -mcpu=neoverse-n2 -march=armv8.4-a shrn.c     
cc1: warning: switch '-mcpu=neoverse-n2' conflicts with '-march=armv8.4-a' switch
#aarch64-linux-gnu-gcc -mcpu=neoverse-n2 -march=armv8.5-a shrn.c
cc1: warning: switch '-mcpu=neoverse-n2' conflicts with '-march=armv8.5-a' switch
#aarch64-linux-gnu-gcc -mcpu=neoverse-n2 -march=armv8.6-a shrn.c
cc1: warning: switch '-mcpu=neoverse-n2' conflicts with '-march=armv8.6-a' switch
#aarch64-linux-gnu-gcc -mcpu=neoverse-n2 -march=armv8.7-a shrn.c
cc1: warning: switch '-mcpu=neoverse-n2' conflicts with '-march=armv8.7-a' switch

> 
> > +                candidate_march = part_number_config['march']
> > +            endif
> >          else
> >              supported_marchs = ['armv8.6-a', 'armv8.5-a', 'armv8.4-a', 'armv8.3-
> a',
> >                                  'armv8.2-a', 'armv8.1-a', 'armv8-a']