diff mbox series

build: use generic march on arm64 when using 'default' machine

Message ID 20181224125627.25690-1-bluca@debian.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series build: use generic march on arm64 when using 'default' machine |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Luca Boccassi Dec. 24, 2018, 12:56 p.m. UTC 
  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

Bruce Richardson Jan. 7, 2019, 12:24 p.m. UTC | #1 
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>
Luca Boccassi Jan. 7, 2019, 1:40 p.m. UTC | #2 
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.
Luca Boccassi Jan. 7, 2019, 1:45 p.m. UTC | #3 
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.
diff mbox series

Patch

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'
 
 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(