Re: [dpdk-dev] [PATCH v3] config/arm: split march cfg into arch and features

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

> -----Original Message-----
> From: fengchengwen <fengchengwen@huawei.com>
> Sent: Friday, July 16, 2021 5:42 AM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Juraj Linkeš
> <juraj.linkes@pantheon.tech>; thomas@monjalon.net;
> david.marchand@redhat.com; bruce.richardson@intel.com; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; ferruh.yigit@intel.com;
> jerinjacobk@gmail.com; jerinj@marvell.com; Pavan Nikhilesh
> <pbhagavatula@marvell.com>
> Cc: dev@dpdk.org; nd <nd@arm.com>
> Subject: Re: [PATCH v3] config/arm: split march cfg into arch and features
> 
> 
> [snip]
> 
> >> +
> >> +    # probe supported marchs and their features
> >> +    candidate_march = ''
> >> +    if part_number_config.has_key('march')
> >> +        supported_marchs = ['armv8.6-a', 'armv8.5-a', 'armv8.4-a', 'armv8.3-a',
> >> +                            'armv8.2-a', 'armv8.1-a', 'armv8-a']
> >> +        check_compiler_support = false
> >> +        foreach supported_march: supported_marchs
> >> +            if supported_march == part_number_config['march']
> >> +                # start checking from this version downwards
> >> +                check_compiler_support = true
> >> +            endif
> >> +            if (check_compiler_support and
> >> +                cc.has_argument('-march=' + supported_march))
> >> +                candidate_march = supported_march
> >> +                # highest supported march version found
> >> +                break
> >> +            endif
> 
> I think breaking it down into two steps is more intuitive.
> step1: find the march version which target config.
>        If not find then error exit.
> step2: start checking from step1's version.
> 

Ok, so you want to move the error check? Meson doesn't have good tools in terms of loops and arrays and if we move the check we'd have to either go through the list twice or have otherwise ugly code. If you know how to implement this neatly then we can change it.

> >> +        endforeach
> >> +        if candidate_march == ''
> >> +            error('No suitable armv8 march version found.')
> >> +        else
> 
> There no need use else, because meson will halt when execute error.
> 

Ok, I'll remove it since this pattern is used elsewhere in DPDK meson.

> >> +            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))
> >> +            endif
> >> +            candidate_march = '-march=' + candidate_march
> >>          endif
> >> -    endforeach
> >> +        if part_number_config.has_key('march_features')
> >> +            feature_unsupported = false
> >> +            foreach feature: part_number_config['march_features']
> >> +                if cc.has_argument('+'.join([candidate_march, feature]))
> >> +                    candidate_march = '+'.join([candidate_march, feature])
> >> +                else
> >> +                    feature_unsupported = true
> >> +                endif
> >> +            endforeach
> >> +            if feature_unsupported
> >> +                warning('Configuration march features are ' +
> >> +                        '@0@, but the compiler supports only @1@.'
> >> +                        .format(part_number_config['march_features'],
> >> +                                candidate_march))
> 
> the march_feature is some like 'crc sve', but candidate_march is '-
> march=armv8.2a+crc'.
> These two displays may be a little weird because later one has -march=armv8.2
> prefix.
> 
> I think it's better move warning to place which feature_unsupported was set
> true.
> 

We'll have more warning this way but it'll result in less confusing messages and nicer code - I'll make the change.

> >> +            endif
> >> +        endif
> >> +        machine_args += candidate_march
> >> +    endif
> >> +
> >> +    # apply supported compiler options
> >> +    if part_number_config.has_key('compiler_options')
> >> +        foreach flag: part_number_config['compiler_options']
> >> +            if cc.has_argument(flag)
> 
> Is it possible that -mcpu= conflicts with -march ?
> 

Maybe, but I think this sort of check is out of scope of this change. We can do this in a separate patch if there's a need (and if it's actually possible to do the check).

> >> +                machine_args += flag
> >> +            else
> >> +                warning('Configuration compiler option ' +
> >> +                        '@0@ isn\'t supported.'.format(flag))
> >> +            endif
> >> +        endforeach
> >> +    endif
> >>
> >>      # apply flags
> >>      foreach flag: dpdk_flags
> >> --
> >> 2.20.1
> >