Re: [dpdk-dev] [PATCH v2] build: check functionality rather than binutils version

2020-07-03 Thread Bruce Richardson 
On Thu, Jul 02, 2020 at 11:28:51PM +0200, Thomas Monjalon wrote:
> 18/06/2020 13:56, Bruce Richardson:
> > Rather than checking the binutils version number, which can lead to
> > unnecessary disabling of AVX512 if fixes have been backported to distro
> > versions, we can instead check the output of "as" from binutils to see if
> > it is correct.
> > 
> > The check in the script uses the minimal assembly reproduction code posted
> > to the public bug tracker for gcc/binutils for those issues [1]. If the
> > binutils bug is present, the instruction parameters - specifically the
> > displacement parameter - will be different in the disassembled output
> > compared to the input. Therefore the check involves assembling a single
> > instruction and disassembling it again, checking that the two match.
> [...]
> > --- /dev/null
> > +++ b/buildtools/binutils-avx512-check.sh
> > +MESON_BUILD_ROOT=${MESON_BUILD_ROOT:-/tmp}
> > +OBJFILE=$MESON_BUILD_ROOT/binutils-avx512-check.o
> > +# from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90028
> > +GATHER_PARAMS='0x8(,%ymm1,1),%ymm0{%k2}'
> > +
> > +# assemble vpgather to file and similarly check
> > +echo "vpgatherqq $GATHER_PARAMS" | $AS --64 -o $OBJFILE -
> > +objdump -d  --no-show-raw-insn $OBJFILE | grep -q $GATHER_PARAMS || {
> > +   echo "vpgatherqq displacement error with as"
> > +   exit 1
> > +}
> 
> For the temporary OBJFILE, please use mktemp and trap for cleanup.
> If you grep "mktemp" in DPDK, you will see the filename is pretty well
> standardized with "dpdk." as prefix.
> Thanks
>
Will change for v3

Re: [dpdk-dev] [PATCH v2] build: check functionality rather than binutils version

2020-07-02 Thread Thomas Monjalon 
18/06/2020 13:56, Bruce Richardson:
> Rather than checking the binutils version number, which can lead to
> unnecessary disabling of AVX512 if fixes have been backported to distro
> versions, we can instead check the output of "as" from binutils to see if
> it is correct.
> 
> The check in the script uses the minimal assembly reproduction code posted
> to the public bug tracker for gcc/binutils for those issues [1]. If the
> binutils bug is present, the instruction parameters - specifically the
> displacement parameter - will be different in the disassembled output
> compared to the input. Therefore the check involves assembling a single
> instruction and disassembling it again, checking that the two match.
[...]
> --- /dev/null
> +++ b/buildtools/binutils-avx512-check.sh
> +MESON_BUILD_ROOT=${MESON_BUILD_ROOT:-/tmp}
> +OBJFILE=$MESON_BUILD_ROOT/binutils-avx512-check.o
> +# from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90028
> +GATHER_PARAMS='0x8(,%ymm1,1),%ymm0{%k2}'
> +
> +# assemble vpgather to file and similarly check
> +echo "vpgatherqq $GATHER_PARAMS" | $AS --64 -o $OBJFILE -
> +objdump -d  --no-show-raw-insn $OBJFILE | grep -q $GATHER_PARAMS || {
> + echo "vpgatherqq displacement error with as"
> + exit 1
> +}

For the temporary OBJFILE, please use mktemp and trap for cleanup.
If you grep "mktemp" in DPDK, you will see the filename is pretty well
standardized with "dpdk." as prefix.
Thanks

Re: [dpdk-dev] [PATCH v2] build: check functionality rather than binutils version

2020-07-02 Thread Van Haaren, Harry 
> -Original Message-
> From: Van Haaren, Harry
> Sent: Thursday, June 18, 2020 1:05 PM
> To: 'Bruce Richardson' ; dev@dpdk.org
> Cc: Yigit, Ferruh ; tho...@monjalon.net; Richardson,
> Bruce 
> Subject: RE: [dpdk-dev] [PATCH v2] build: check functionality rather than 
> binutils
> version
> 
> > -Original Message-
> > From: dev  On Behalf Of Bruce Richardson
> > Sent: Thursday, June 18, 2020 12:57 PM
> > To: dev@dpdk.org
> > Cc: Yigit, Ferruh ; tho...@monjalon.net; Richardson,
> > Bruce 
> > Subject: [dpdk-dev] [PATCH v2] build: check functionality rather than 
> > binutils
> > version
> >
> > Rather than checking the binutils version number, which can lead to
> > unnecessary disabling of AVX512 if fixes have been backported to distro
> > versions, we can instead check the output of "as" from binutils to see if
> > it is correct.
> >
> > The check in the script uses the minimal assembly reproduction code posted
> > to the public bug tracker for gcc/binutils for those issues [1]. If the
> > binutils bug is present, the instruction parameters - specifically the
> > displacement parameter - will be different in the disassembled output
> > compared to the input. Therefore the check involves assembling a single
> > instruction and disassembling it again, checking that the two match.
> >
> > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90028
> >
> > Signed-off-by: Bruce Richardson 
> 
> Tested on binutils 2.30 without backported fixes, can confirm that __AVX512F__
> define is
> not present at meson configure time.

Re-tested with Ubuntu LTS version 18.04 with the proposed binutils backport fix:
https://bugs.launchpad.net/ubuntu/+source/binutils/+bug/1883880

With this fix, AVX512 gets enabled as it should after running the check:
Fetching value of define "__AVX512F__": 1

As before, still, and now even more so:
Tested-by: Harry van Haaren

Re: [dpdk-dev] [PATCH v2] build: check functionality rather than binutils version

2020-06-18 Thread Van Haaren, Harry 
> -Original Message-
> From: dev  On Behalf Of Bruce Richardson
> Sent: Thursday, June 18, 2020 12:57 PM
> To: dev@dpdk.org
> Cc: Yigit, Ferruh ; tho...@monjalon.net; Richardson,
> Bruce 
> Subject: [dpdk-dev] [PATCH v2] build: check functionality rather than binutils
> version
> 
> Rather than checking the binutils version number, which can lead to
> unnecessary disabling of AVX512 if fixes have been backported to distro
> versions, we can instead check the output of "as" from binutils to see if
> it is correct.
> 
> The check in the script uses the minimal assembly reproduction code posted
> to the public bug tracker for gcc/binutils for those issues [1]. If the
> binutils bug is present, the instruction parameters - specifically the
> displacement parameter - will be different in the disassembled output
> compared to the input. Therefore the check involves assembling a single
> instruction and disassembling it again, checking that the two match.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90028
> 
> Signed-off-by: Bruce Richardson 

Tested on binutils 2.30 without backported fixes, can confirm that __AVX512F__ 
define is
not present at meson configure time.

Tested-by: Harry van Haaren

[dpdk-dev] [PATCH v2] build: check functionality rather than binutils version

2020-06-18 Thread Bruce Richardson 
Rather than checking the binutils version number, which can lead to
unnecessary disabling of AVX512 if fixes have been backported to distro
versions, we can instead check the output of "as" from binutils to see if
it is correct.

The check in the script uses the minimal assembly reproduction code posted
to the public bug tracker for gcc/binutils for those issues [1]. If the
binutils bug is present, the instruction parameters - specifically the
displacement parameter - will be different in the disassembled output
compared to the input. Therefore the check involves assembling a single
instruction and disassembling it again, checking that the two match.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90028

Signed-off-by: Bruce Richardson 

---
V2:
- Renamed "as_ok" variable to "binutils_ok" for readability
- Removed one test case from the script because even though two DPDK bugs
  were filed, the one binutils bug is the root cause, and a single commit
  fixes them both
- Changed message() to warning() in the printout
---
 buildtools/binutils-avx512-check.sh | 16 
 buildtools/meson.build  |  3 +--
 config/x86/meson.build  | 10 +++---
 meson.build |  5 -
 4 files changed, 24 insertions(+), 10 deletions(-)
 create mode 100755 buildtools/binutils-avx512-check.sh

diff --git a/buildtools/binutils-avx512-check.sh 
b/buildtools/binutils-avx512-check.sh
new file mode 100755
index 0..cef7b09ed
--- /dev/null
+++ b/buildtools/binutils-avx512-check.sh
@@ -0,0 +1,16 @@
+#! /bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2020 Intel Corporation
+
+AS=${AS:-as}
+MESON_BUILD_ROOT=${MESON_BUILD_ROOT:-/tmp}
+OBJFILE=$MESON_BUILD_ROOT/binutils-avx512-check.o
+# from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90028
+GATHER_PARAMS='0x8(,%ymm1,1),%ymm0{%k2}'
+
+# assemble vpgather to file and similarly check
+echo "vpgatherqq $GATHER_PARAMS" | $AS --64 -o $OBJFILE -
+objdump -d  --no-show-raw-insn $OBJFILE | grep -q $GATHER_PARAMS || {
+   echo "vpgatherqq displacement error with as"
+   exit 1
+}
diff --git a/buildtools/meson.build b/buildtools/meson.build
index d5f8291be..73a847c52 100644
--- a/buildtools/meson.build
+++ b/buildtools/meson.build
@@ -1,13 +1,12 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2017-2019 Intel Corporation
 
-subdir('pmdinfogen')
-
 pkgconf = find_program('pkg-config', 'pkgconf', required: false)
 pmdinfo = find_program('gen-pmdinfo-cfile.sh')
 list_dir_globs = find_program('list-dir-globs.py')
 check_symbols = find_program('check-symbols.sh')
 ldflags_ibverbs_static = find_program('options-ibverbs-static.sh')
+binutils_avx512_check = find_program('binutils-avx512-check.sh')
 
 # set up map-to-def script using python, either built-in or external
 python3 = import('python').find_installation(required: false)
diff --git a/config/x86/meson.build b/config/x86/meson.build
index adc857ba2..6ec020ef6 100644
--- a/config/x86/meson.build
+++ b/config/x86/meson.build
@@ -3,14 +3,10 @@
 
 # get binutils version for the workaround of Bug 97
 if not is_windows
-   ldver = run_command('ld', '-v').stdout().strip()
-   if ldver.contains('2.30') and cc.has_argument('-mno-avx512f')
+   binutils_ok = run_command(binutils_avx512_check)
+   if binutils_ok.returncode() != 0 and cc.has_argument('-mno-avx512f')
machine_args += '-mno-avx512f'
-   message('Binutils 2.30 detected, disabling AVX512 support as 
workaround for bug #97')
-   endif
-   if ldver.contains('2.31') and cc.has_argument('-mno-avx512f')
-   machine_args += '-mno-avx512f'
-   message('Binutils 2.31 detected, disabling AVX512 support as 
workaround for bug #249')
+   warning('Binutils error with AVX512 assembly, disabling AVX512 
support')
endif
 endif
 
diff --git a/meson.build b/meson.build
index 750fdeab1..ab4d60cfa 100644
--- a/meson.build
+++ b/meson.build
@@ -41,10 +41,13 @@ global_inc = include_directories('.', 'config',
'lib/librte_eal/@0@/include'.format(host_machine.system()),
'lib/librte_eal/@0@/include'.format(arch_subdir),
 )
+
+# do configuration and get tool paths
+subdir('buildtools')
 subdir('config')
 
 # build libs and drivers
-subdir('buildtools')
+subdir('buildtools/pmdinfogen')
 subdir('lib')
 subdir('drivers')
 
-- 
2.25.1