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