RE: [PATCH v3 1/3] config/arm: avoid mcpu and march conflicts

[Wathsala Wathawana Vithanage <wathsala.vithanage@arm.com>]

> > <wathsala.vithanage@arm.com> wrote:
> > >
> > > Hi Pavan,
> > >
> > >> 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.
> > >
> > > Why would one force the march specified in the part number when mcpu
> > > for that part number is also available and supported by the compiler?
> > >
> > It would be good to explain the use case or the problem being faced.
> >
> 
> The idea of this patchset is to avoid mcpu and march conflicts that can happen
> with the current build flow.
> 
> #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'
> 
> In some versions of GCC mcpu=neoverse-n2 is supported but -march=armv9-
> a is not so, current build flow will choose the next supported march which is
> armv8.6-a and report a conflict.
> 
If compiler support is available for a certain CPU, then it is safe to assume that the
Compiler knows the best architecture to use.
Therefore, in such cases the best practice is to not provide -march.

> > >>
> > >> Example:
> > >> march = armv9-a
> > >> mcpu = neoverse-n2
> > >>
> > >> mcpu supported, march supported
> > >> machine_args = ['-mcpu=neoverse-n2', '-march=armv9-a']
> > >
> > > -march restricts the compiler to baseline architecture of the -mcpu.
> > > For instance, Neoverse-n1's baseline architecture is armv8.2-a, but
> > > it has some extensions from armv8.3-a, armv8.4-a, and armv8.5-a.
> > > By setting -march to armv8.2-a the compiler will strictly omit
> > > extensions from 8.3, 8.4 and 8.5 resulting in a suboptimal outcome.
> 
> What if compiler only supports armv8.2-a?
> Are you suggesting we don’t use march at all when mcpu is supported?
> If so how do you express extensions that the SoC supports?
> Neoverse-n2 has optional support for crypto and can only be enabled by
> expressing it through march='armv9-a+crypto'
> 
March extensions also works with mcpu, use mcpu=neoverse-n2+crypto
instead of march.  It's documented in "-march and -mcpu Feature Modifiers"
section in gcc manual.

> > >
> > >>
> > >> mcpu supported, march not supported machine_args =
> > >> ['-mcpu=neoverse-n2']
> > >
> > > This will result in the best outcome.
> 
> Isn't -mcpu=neoverse-n2 -march=armv9-a+sve2+crypto also the best
> outcome?
> 
Here also we can append feature modifiers like sve2 and crypto to CPU in
-mcpu and drop -march arg.
If the compiler supports neoverse-n2 but not armv9-a it will pick the next
best architecture. 
-mcpu=neoverse-n2+sve2+crypto can replace - march=armv9-a+sve2+crypto

> > >
> > >>
> > >> mcpu not supported, march supported machine_args =
> > >> ['-march=armv9-a']
> > >
> > > This too may result in a suboptimal outcome as optimization space is
> > > limited to the given march (not using extensions from later
> > > architectures when available).
> > >
> 
> What if compiler doesn’t support mcpu=neoverse-n2 and only supports
> march=armv9-a
> 
I agree there can be such corner cases where CPU enablement isn't done.
Such cases can be handled with a new meson build parameter like 
-Dplatform=generic-armv9 to build armv9-a binaries (similar to 
-Dplatform=generic that builds armv8-a binaries today).
Having such parameter forces the user to make a conscious decision rather 
than build system doing it for them.
It also comes with the added benefit of having a simpler build system.

> > >>
> > >> mcpu not supported, march not supported machine_args =
> > >> ['-march=armv8.6-a']
> > >
> > > Compiler knows nothing about the target CPU or the architecture.
> > > I think it's better to exit the build process with an error.
> > >
> 
> Then we would need to mark all old GCC versions as not supported by a newer
> SoC I don’t think that’s needed since the binaries still run but not optimally,
> currently we have a warning in place for march mismatch.
> 
We don't have to deprecate older versions of GCC.
I'm suggesting two options to let the user have greater autonomy on
the kind of the binary they want rather than ending up with a binary
the build system forced on them.
Today -Dplatform=generic does something similar to this with armv8,
it simply directs the build system to output an armv8 without any extras. 
First suggestion is that we simply have a -Dplatform=generic-armv9 that
does the same but for armv9.
The second suggestion is to empower a sophisticated user even further
to override everything in the build system including generics via two 
parameters -Dmarch and -Dmcpu to set an arbitrary architecture and a cpu.
Second option works as a catch-all for every unorthodox request that may
come our way. Both these features can be suggested when build exits due
to compiler not knowing the target CPU or the architecture.
I think these parameters keep user in charge with a simpler build system.

> > >>
> > >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > >> ---
> > >> v2 Changes:
> > >> - Cleanup march inconsistencies. (Juraj Linkes)
> > >> - Unify fallback march selection. (Juraj Linkes)
> > >> - Tag along ARM WFE patch.
> > >> v3 Changes:
> > >> - Fix missing 'fallback_march' key check.
> > >>
> > >> config/arm/meson.build | 108 +++++++++++++++++++++++++-----------
> --
> > --
> > >> -
> > >> 1 file changed, 66 insertions(+), 42 deletions(-)
> > >>
> > >> diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > >> 36f21d22599a..ba859bd060b5 100644
> > >> --- a/config/arm/meson.build
> > >> +++ b/config/arm/meson.build
> > >> @@ -58,18 +58,18 @@ implementer_generic = {  }
> > >>
> > >> part_number_config_arm = {
> > >> -    '0xd03': {'compiler_options':  ['-mcpu=cortex-a53']},
> > >> -    '0xd04': {'compiler_options':  ['-mcpu=cortex-a35']},
> > >> -    '0xd05': {'compiler_options':  ['-mcpu=cortex-a55']},
> > >> -    '0xd07': {'compiler_options':  ['-mcpu=cortex-a57']},
> > >> -    '0xd08': {'compiler_options':  ['-mcpu=cortex-a72']},
> > >> -    '0xd09': {'compiler_options':  ['-mcpu=cortex-a73']},
> > >> -    '0xd0a': {'compiler_options':  ['-mcpu=cortex-a75']},
> > >> -    '0xd0b': {'compiler_options':  ['-mcpu=cortex-a76']},
> > >> +    '0xd03': {'mcpu': 'cortex-a53'},
> > >> +    '0xd04': {'mcpu': 'cortex-a35'},
> > >> +    '0xd05': {'mcpu': 'cortex-a55'},
> > >> +    '0xd07': {'mcpu': 'cortex-a57'},
> > >> +    '0xd08': {'mcpu': 'cortex-a72'},
> > >> +    '0xd09': {'mcpu': 'cortex-a73'},
> > >> +    '0xd0a': {'mcpu': 'cortex-a75'},
> > >> +    '0xd0b': {'mcpu': 'cortex-a76'},
> > >>     '0xd0c': {
> > >>         'march': 'armv8.2-a',
> > >>         'march_features': ['crypto', 'rcpc'],
> > >> -        'compiler_options':  ['-mcpu=neoverse-n1'],
> > >> +        'mcpu': 'neoverse-n1',
> > >>         'flags': [
> > >>             ['RTE_MACHINE', '"neoverse-n1"'],
> > >>             ['RTE_ARM_FEATURE_ATOMICS', true], @@ -81,7 +81,7 @@
> > >> part_number_config_arm = {
> > >>     '0xd40': {
> > >>         'march': 'armv8.4-a',
> > >>         'march_features': ['sve'],
> > >> -        'compiler_options':  ['-mcpu=neoverse-v1'],
> > >> +        'mcpu': 'neoverse-v1',
> > >>         'flags': [
> > >>             ['RTE_MACHINE', '"neoverse-v1"'],
> > >>             ['RTE_ARM_FEATURE_ATOMICS', true], @@ -92,8 +92,9 @@
> > >> part_number_config_arm = {
> > >>         'march': 'armv8.4-a',
> > >>     },
> > >>     '0xd49': {
> > >> +        'march': 'armv9-a',
> > >>         'march_features': ['sve2'],
> > >> -        'compiler_options': ['-mcpu=neoverse-n2'],
> > >> +        'mcpu': 'neoverse-n2',
> > >>         'flags': [
> > >>             ['RTE_MACHINE', '"neoverse-n2"'],
> > >>             ['RTE_ARM_FEATURE_ATOMICS', true], @@ -127,21 +128,23
> > >> @@ 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'],
> > >> +            'march': 'armv8-a',
> > >> +            'march_features': ['crc'],
> > >> +            'mcpu': 'thunderxt83',
> > >>             '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 +156,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 +179,7
> > >> @@ implementer_ampere = {
> > >>         '0x0': {
> > >>             'march': 'armv8-a',
> > >>             'march_features': ['crc', 'crypto'],
> > >> -            'compiler_options':  ['-mtune=emag'],
> > >> +            'mcpu': 'emag',
> > >>             'flags': [
> > >>                 ['RTE_MACHINE', '"eMAG"'],
> > >>                 ['RTE_MAX_LCORE', 32], @@ -186,7 +189,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 +209,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
> > >> +698,21 @@
> > if
> > >> update_flags
> > >>
> > >>     machine_args = [] # Clear previous machine args
> > >>
> > >> +    candidate_mcpu = ''
> > >> +    if part_number_config.has_key('mcpu')
> > >> +        mcpu = part_number_config['mcpu']
> > >> +        if (cc.has_argument('-mcpu=' + mcpu))
> > >> +            candidate_mcpu = mcpu
> > >> +        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
> > >> + candidate_mcpu !=
> > ''
> > >> +            if cc.has_argument('-march=' +  part_number_config['march'])
> > >> +                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'] @@ -717,32 +730,43 @@ if update_flags
> > >>                 endif
> > >>             endforeach
> > >>         endif
> > >> -        if candidate_march == ''
> > >> -            error('No suitable armv8 march version found.')
> > >> -        endif
> > >> +
> > >>         if candidate_march != part_number_config['march']
> > >> -            warning('Configuration march version is ' +
> > >> -                    '@0@, but the compiler supports only @1@.'
> > >> -                    .format(part_number_config['march'], candidate_march))
> > >> +            warning('Configuration march version is @0@, not supported.'
> > >> +                    .format(part_number_config['march']))
> > >> +            if candidate_march != ''
> > >> +                warning('Using march version @0@.'.format(candidate_march))
> > >> +            endif
> > >>         endif
> > >> -        candidate_march = '-march=' + candidate_march
> > >>
> > >> -        march_features = []
> > >> -        if part_number_config.has_key('march_features')
> > >> -            march_features += part_number_config['march_features']
> > >> -        endif
> > >> -        if soc_config.has_key('extra_march_features')
> > >> -            march_features += soc_config['extra_march_features']
> > >> +        if candidate_march == '' and candidate_mcpu == ''
> > >> +            error('No suitable ARM march/mcpu version found.')
> > >>         endif
> > >> -        foreach feature: march_features
> > >> -            if cc.has_argument('+'.join([candidate_march, feature]))
> > >> -                candidate_march = '+'.join([candidate_march, feature])
> > >> -            else
> > >> -                warning('The compiler does not support feature @0@'
> > >> -                    .format(feature))
> > >> +
> > >> +        if candidate_march != ''
> > >> +            candidate_march = '-march=' + candidate_march
> > >> +            march_features = []
> > >> +            if part_number_config.has_key('march_features')
> > >> +                march_features +=
> > >> + part_number_config['march_features']
> > >>             endif
> > >> -        endforeach
> > >> -        machine_args += candidate_march
> > >> +            if soc_config.has_key('extra_march_features')
> > >> +                march_features += soc_config['extra_march_features']
> > >> +            endif
> > >> +            foreach feature: march_features
> > >> +                if cc.has_argument('+'.join([candidate_march, feature]))
> > >> +                    candidate_march = '+'.join([candidate_march, feature])
> > >> +                else
> > >> +                    warning('The compiler does not support feature @0@'
> > >> +                        .format(feature))
> > >> +                endif
> > >> +            endforeach
> > >> +            machine_args += candidate_march
> > >> +        endif
> > >> +    endif
> > >> +
> > >> +    if candidate_mcpu != ''
> > >> +        candidate_mcpu = '-mcpu=' + candidate_mcpu
> > >> +        machine_args += candidate_mcpu
> > >>     endif
> > >>
> > >>     # apply supported compiler options
> > >> --
> > >> 2.43.0
> > >