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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>
> 
> On Wed, May 19, 2021 at 6:58 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.1-a', '-march=armv8.2-a'] 2. Define
> > 'march_feature' tuple which defines support feature.
> > e.g. 'march_feature': ['crypto', 'sve']
> > Note: If user defined 'march_feature', it also needs to define a valid
> > 'march_base' because 'march_feature' depends on 'march_base' when
> > checking validity.
> > 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 | 31 +++++++++++++++++++++++++++++--
> >  1 file changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > 3f34ec9..044812f 100644
> > --- a/config/arm/meson.build
> > +++ b/config/arm/meson.build
> > @@ -158,7 +158,9 @@ implementer_hisilicon = {
> >              ]
> >          },
> >          '0xd02': {
> > -            'machine_args': ['-march=armv8.2-a+crypto+sve'],
> > +            'march_base': ['-march=armv8.2-a'],
> > +            'march_feature': ['crypto', 'sve'],
> > +            'machine_args': [],
> 
> IMO, This patch must split into two as this problem is not specific to kunpeng
> patch 1: config/arm: infrastructure to add most suitable -march or so(This will
> have only infra) patch 2: config/arm: update the targets to support most
> suitable -march or so(This one will one will have changes for all targets
> including kunpeng)
> 
> 
> 
> 
> >              'flags': [
> >                  ['RTE_MACHINE', '"Kunpeng 930"'],
> >                  ['RTE_ARM_FEATURE_ATOMICS', true], @@ -449,8 +451,33
> > @@ 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')
> > +        tmp_machine_march = ''
> > +        march_valid = false
> > +        foreach march: part_number_config['march_base']
> > +            if cc.has_argument(march)
> > +                tmp_machine_march = march # Set the higher supported march
> as possible
> > +                march_valid = true
> > +            endif
> > +        endforeach
> > +        # select feature only when march valid
> > +        if march_valid and
> > + part_number_config.has_key('march_feature')
> 
> I think, in order to make more generic, IMO, it should be
> 1) List all the march and features options(crypto, sve)
> 2) Probe the compiler to find the supported march, features
> 3) In each target,
> - define the minimum march and features
I think we should list the max march supported by the target. We can use that to go back in decreasing order to identify the march the compiler supports.
We would need a list of features between those marchs + any additional features the target supports. We can probe those features in compiler to add the support for those features.
I am thinking this will not require us to throw an error.


> - define the the best need march and features
> 4) If the compiler support the best config, pick that else
> 5) If the compiler supports the minimum config, pick that else
> 6) Don't build and throw the error.
> 
> 
> > +            foreach feature: part_number_config['march_feature']
> > +                tmp_feature = tmp_machine_march + '+' + feature
> > +                if cc.has_argument(tmp_feature)
> > +                    tmp_machine_march = tmp_feature # Set the more supported
> feature as possible
> > +                endif
> > +            endforeach
> > +        endif
> > +        if march_valid
> > +            machine_march = [tmp_machine_march]
> > +        endif
> > +    endif
> > +
> >      # apply supported machine args
> > -    machine_args = [] # Clear previous machine args
> > +    machine_args = machine_march # Init with machine march which set
> > + above
> >      foreach flag: part_number_config['machine_args']
> >          if cc.has_argument(flag)
> >              machine_args += flag
> > --
> > 2.8.1
> >