build: use generic march on arm64 when using 'default' machine
Checks
Commit Message
When building for generic distribution we need a stable baseline
architecture, or depending on the build worker the result will vary.
Force the default flags if the user explicitly sets marchine=default
at configuration time.
Fixes: b1d48c41189a ("build: support ARM with meson")
Cc: stable@dpdk.org
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
config/arm/meson.build | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
Comments
On Mon, Dec 24, 2018 at 01:56:27PM +0100, Luca Boccassi wrote:
> When building for generic distribution we need a stable baseline
> architecture, or depending on the build worker the result will vary.
>
> Force the default flags if the user explicitly sets marchine=default
typo: marchine
> at configuration time.
>
> Fixes: b1d48c41189a ("build: support ARM with meson")
> Cc: stable@dpdk.org
>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
> config/arm/meson.build | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/config/arm/meson.build b/config/arm/meson.build
> index dae55d6b2..fa21a2fd2 100644
> --- a/config/arm/meson.build
> +++ b/config/arm/meson.build
> @@ -6,6 +6,7 @@
> march_opt = '-march=@0@'.format(machine)
>
> arm_force_native_march = false
> +arm_force_default_march = machine == 'default'
Do we need a new variable here? Given it only seems to be used once below,
I think just having the boolean expression directly in the if statement is
clearer. If you do keep the variable, suggest putting braces around the
comparison, otherwise at first glance it looks like a chained assignment
like you get in C e.g. x = y = 0;
>
> machine_args_generic = [
> ['default', ['-march=armv8-a+crc+crypto']],
> @@ -105,7 +106,11 @@ else
> cmd_generic = ['generic', '', '', 'default', '']
> cmd_output = cmd_generic # Set generic by default
> machine_args = [] # Clear previous machine args
> - if not meson.is_cross_build()
> + if arm_force_default_march and not meson.is_cross_build()
> + machine = impl_generic
> + cmd_output = cmd_generic
> + impl_pn = 'default'
> + elif not meson.is_cross_build()
> # The script returns ['Implementer', 'Variant', 'Architecture',
> # 'Primary Part number', 'Revision']
> detect_vendor = find_program(join_paths(
> --
> 2.19.2
>
With these comments, looks ok to me from a meson viewpoint. I think an ack
from the arm side would be good to get too though.
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
On Mon, 2019-01-07 at 12:24 +0000, Bruce Richardson wrote:
> On Mon, Dec 24, 2018 at 01:56:27PM +0100, Luca Boccassi wrote:
> > When building for generic distribution we need a stable baseline
> > architecture, or depending on the build worker the result will
> > vary.
> >
> > Force the default flags if the user explicitly sets
> > marchine=default
>
> typo: marchine
>
> > at configuration time.
> >
> > Fixes: b1d48c41189a ("build: support ARM with meson")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> > config/arm/meson.build | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/config/arm/meson.build b/config/arm/meson.build
> > index dae55d6b2..fa21a2fd2 100644
> > --- a/config/arm/meson.build
> > +++ b/config/arm/meson.build
> > @@ -6,6 +6,7 @@
> > march_opt = '-march=@0@'.format(machine)
> >
> > arm_force_native_march = false
> > +arm_force_default_march = machine == 'default'
>
> Do we need a new variable here? Given it only seems to be used once
> below,
> I think just having the boolean expression directly in the if
> statement is
> clearer. If you do keep the variable, suggest putting braces around
> the
> comparison, otherwise at first glance it looks like a chained
> assignment
> like you get in C e.g. x = y = 0;
>
> >
> > machine_args_generic = [
> > ['default', ['-march=armv8-a+crc+crypto']],
> > @@ -105,7 +106,11 @@ else
> > cmd_generic = ['generic', '', '', 'default', '']
> > cmd_output = cmd_generic # Set generic by default
> > machine_args = [] # Clear previous machine args
> > - if not meson.is_cross_build()
> > + if arm_force_default_march and not meson.is_cross_build()
> > + machine = impl_generic
> > + cmd_output = cmd_generic
> > + impl_pn = 'default'
> > + elif not meson.is_cross_build()
> > # The script returns ['Implementer', 'Variant',
> > 'Architecture',
> > # 'Primary Part number', 'Revision']
> > detect_vendor = find_program(join_paths(
> > --
> > 2.19.2
> >
>
> With these comments, looks ok to me from a meson viewpoint. I think
> an ack
> from the arm side would be good to get too though.
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Fixed typo, removed variable and added the arm maintainers to CC in v2,
thanks.
On Mon, 2019-01-07 at 12:24 +0000, Bruce Richardson wrote:
> On Mon, Dec 24, 2018 at 01:56:27PM +0100, Luca Boccassi wrote:
> > When building for generic distribution we need a stable baseline
> > architecture, or depending on the build worker the result will
> > vary.
> >
> > Force the default flags if the user explicitly sets
> > marchine=default
>
> typo: marchine
>
> > at configuration time.
> >
> > Fixes: b1d48c41189a ("build: support ARM with meson")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> > config/arm/meson.build | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/config/arm/meson.build b/config/arm/meson.build
> > index dae55d6b2..fa21a2fd2 100644
> > --- a/config/arm/meson.build
> > +++ b/config/arm/meson.build
> > @@ -6,6 +6,7 @@
> > march_opt = '-march=@0@'.format(machine)
> >
> > arm_force_native_march = false
> > +arm_force_default_march = machine == 'default'
>
> Do we need a new variable here? Given it only seems to be used once
> below,
> I think just having the boolean expression directly in the if
> statement is
> clearer. If you do keep the variable, suggest putting braces around
> the
> comparison, otherwise at first glance it looks like a chained
> assignment
> like you get in C e.g. x = y = 0;
Eheh it looks like I was a bit too hasty - I now remember that the main
reason I added a new variable is that the "machine" variable gets
overridden just before the if branch, so the original value is lost. I
could refactor and rename, but that would be more intrusive so I had
opted to just do what was already done for the other "force" case.
@@ -6,6 +6,7 @@
march_opt = '-march=@0@'.format(machine)
arm_force_native_march = false
+arm_force_default_march = machine == 'default'
machine_args_generic = [
['default', ['-march=armv8-a+crc+crypto']],
@@ -105,7 +106,11 @@ else
cmd_generic = ['generic', '', '', 'default', '']
cmd_output = cmd_generic # Set generic by default
machine_args = [] # Clear previous machine args
- if not meson.is_cross_build()
+ if arm_force_default_march and not meson.is_cross_build()
+ machine = impl_generic
+ cmd_output = cmd_generic
+ impl_pn = 'default'
+ elif not meson.is_cross_build()
# The script returns ['Implementer', 'Variant', 'Architecture',
# 'Primary Part number', 'Revision']
detect_vendor = find_program(join_paths(