Re: [dpdk-dev] [PATCH v4 1/2] config/arm: select most suitable -march for kunpeng soc

[Jerin Jacob <jerinjacobk@gmail.com>]

On Tue, May 18, 2021 at 6:55 PM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
>
> <snip>
>
> > >>
> > >> On Thu, May 13, 2021 at 6:41 PM Chengwen Feng
> > >> <fengchengwen@huawei.com> wrote:
> > >>>
> > >>> Currently, the soc_kunpeng930 declares
> > >>> '-march=armv8.2-a+crypto+sve', but some compiler doesn't recognize
> > >>> the march because it doesn't support sve.
> > >>>
> > >>> To solve this bug we use the following scheme:
> > >>> 1. Define 'march_base' tuple which defines support march, it should
> > >>> arrange from lower to higher.
> > >>> e.g. 'march_base' : ['-march=armv8-a', '-march=armv8.2-a'] 2. Define
> > >>> 'march_feature' tuple which defines support feature.
> > >>> e.g. 'march_feature' : ['crypto', 'sve'] 3. Select the most suitable
> > >>> march+feature combination based on 'march_base' and 'march_feature'
> > >>> tuples.
> > >>> 4. Use the selected march+feature combination as the default
> > >>> machine_args.
> > >>>
> > >>> Fixes: 7cf32a22b240 ("config/arm: add Hisilicon kunpeng")
> > >>>
> > >>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > >>> ---
> > >>>  config/arm/meson.build | 27 +++++++++++++++++++++++++--
> > >>>  1 file changed, 25 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > >>> 3f34ec9..8551a80 100644
> > >>> --- a/config/arm/meson.build
> > >>> +++ b/config/arm/meson.build
> > >>> @@ -149,7 +149,9 @@ implementer_hisilicon = {
> > >>>      ],
> > >>>      'part_number_config': {
> > >>>          '0xd01': {
> > >>> -            'machine_args': ['-march=armv8.2-a+crypto', '-mtune=tsv110'],
> > >>> +            'march_base' : ['-march=armv8-a', '-march=armv8.2-a'],
> > > If the compiler supports armv8.1-a, you need to choose armv8.1-a.
> > >
> > >>
> > >> Another target has the same issue. Could you fix that all together as
> > >> it is generic problem in the existing infrastructure.
> > > I think this needs to be more generic solution. IMO, the requirement is as
> > follows:
> > >
> > > 1) We need to pick the most closest march_base supported by the
> > > compiler. For ex: If the SoC support v8.2 and the compiler supports
> > > v8.1, we need to pick v8.1
> > > 2) SoCs are allowed to support a base march version + hand picked features
> > from the next 1 base marchs. i.e. armv8.x compliant implementation can
> > include any features from armv8.(x + 1). Please refer to  Arm-ARM section A2
> > for more details. So, it is possible that the compiler supports a base march
> > and a bunch of optional features from the next version. We need to test all
> > the features allowed by the architecture and pick the ones that are supported
> > in the compiler.
> > >
> >
> > I try to add 'march_base' : ['-march=armv8-a', '-march=armv8.5-a'] to cn10k,
> > and then find it can't work with ['RTE_ARM_FEATURE_ATOMICS', true] when
> > using gcc7.2:
> >       [268/2250] Compiling C object
> > lib/librte_stack.a.p/stack_rte_stack_lf.c.o
> >       FAILED: lib/librte_stack.a.p/stack_rte_stack_lf.c.o
> >       ...
> >       {standard input}: Assembler messages:
> >       {standard input}:13: Error: selected processor does not support `caspl
> > x0,x1,x2,x3,[x5]'
> >       [347/2250] Compiling C object
> > lib/librte_hash.a.p/hash_rte_cuckoo_hash.c.o
> >       ninja: build stopped: subcommand failed.
> >       make: *** [cn10k] Error 1
> > It seem can't simplely add '-march=armv8-a' in 'march_base'.
> > And it require a lot of testing to get the right 'march_base' and
> > 'march_feature' parameters.
> > So for v5, I just modify the kunpeng930's config which was well tested.
> >
> > I think the 'march_base' and 'march_feature' could well solve the gcc's minor-
> > version problem.
> > Note: the minor-version means a few version which are closes, not big gap,
> > like gcc5.4 and gcc10.2
> >
> > For that old gcc which could not support the 'march' that defined in
> > 'machine_args' or 'march_base'
> > I think it better use the 'generic' profile else it will compile fail which showed
> > above.
> >
> > So how about add new tuple: fallback_generic? eg:
> >       '0xd02': {
> >             'march_base': ['-march=armv8.2-a'],
> >             'march_feature': ['crypto', 'sve'],
> >             'machine_args': [],
> >             'flags': [
> >                 ['RTE_MACHINE', '"Kunpeng 930"'],
> >                 ['RTE_ARM_FEATURE_ATOMICS', true],
> >                 ['RTE_MAX_LCORE', 1280],
> >                 ['RTE_MAX_NUMA_NODES', 16]
> >             ],
> >           'fallback_generic': true
> >         }
> > PS: The premise is that the 'generic' profile is tested.
> Jerin, how big of a problem is this (i.e. having to compile the code with an older version of the compiler)? Is it just one old version of the compiler or there are several of them? Do they all need to be captured in the meson.build file? I am just trying to understand the need for a generic approach.

IMO, We are supporting from gcc 4.7. So a lot of combinations are possible.


>
> >
> >
> > >>
> > >>
> > >>> +            'march_feature' : ['crypto'],
> > >>> +            'machine_args': ['-mtune=tsv110'],
> > >>>              'flags': [
> > >>>                  ['RTE_MACHINE', '"Kunpeng 920"'],
> > >>>                  ['RTE_ARM_FEATURE_ATOMICS', true], @@ -158,7 +160,9
> > >>> @@ implementer_hisilicon = {
> > >>>              ]
> > >>>          },
> > >>>          '0xd02': {
> > >>> -            'machine_args': ['-march=armv8.2-a+crypto+sve'],
> > >>> +            'march_base' : ['-march=armv8-a', '-march=armv8.2-a'],
> > >>> +            'march_feature' : ['crypto', 'sve'],
> > >>> +            'machine_args': [],
> > >>>              'flags': [
> > >>>                  ['RTE_MACHINE', '"Kunpeng 930"'],
> > >>>                  ['RTE_ARM_FEATURE_ATOMICS', true], @@ -449,8
> > >>> +453,27 @@ else
> > >>>      # add/overwrite flags in the proper order
> > >>>      dpdk_flags = flags_common + implementer_config['flags'] +
> > >>> part_number_config.get('flags', []) + soc_flags
> > >>>
> > >>> +    # select the most suitable march+feature combination
> > >>> +    machine_march = []
> > >>> +    if part_number_config.has_key('march_base')
> > >>> +        foreach arch: part_number_config['march_base']
> > >>> +            if cc.has_argument(arch)
> > >>> +                machine_march = arch # Set the higher supported
> > >>> + arch as
> > >> possible
> > >>> +            endif
> > >>> +        endforeach
> > >>> +    endif
> > >>> +    if part_number_config.has_key('march_feature')
> > >>> +        foreach feature: part_number_config['march_feature']
> > >>> +            tmp_machine_march = machine_march + '+' + feature
> > >>> +            if cc.has_argument(tmp_machine_march)
> > >>> +                machine_march = tmp_machine_march # Set the more
> > >>> + supported
> > >> feature as possible
> > >>> +            endif
> > >>> +        endforeach
> > >>> +    endif
> > >>> +
> > >>>      # apply supported machine args
> > >>>      machine_args = [] # Clear previous machine args
> > >>> +    machine_args += machine_march
> > >>>      foreach flag: part_number_config['machine_args']
> > >>>          if cc.has_argument(flag)
> > >>>              machine_args += flag
> > >>> --
> > >>> 2.8.1
> > >>>
>