This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/4] Add target descriptor for avx-avx512.
- From: Pedro Alves <palves at redhat dot com>
- To: Michael Sturm <michael dot sturm at intel dot com>, mark dot kettenis at xs4all dot nl, eliz at gnu dot org
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 27 May 2016 12:04:14 +0100
- Subject: Re: [PATCH 3/4] Add target descriptor for avx-avx512.
- Authentication-results: sourceware.org; auth=none
- References: <1463143833-24399-1-git-send-email-michael dot sturm at intel dot com> <1463143833-24399-4-git-send-email-michael dot sturm at intel dot com>
s/descriptor/description/g
On 05/13/2016 01:50 PM, Michael Sturm wrote:
> Add a dedicated target descriptor for the feature combination
> avx-avx512 as implemented by certain IA CPU models.
>
> The corresponding X86_XSTATE_AVX_AVX512_MASK already exists, but shared
> the tdesc with X86_XSTATE_AVX_MPX_AVX512_MASK. This caused MPX registers
> displayed as undefined on CPUs that only implemented
> X86_XSTATE_AVX_AVX512_MASK, which is undesired. This patch solves this issue.
>
> This patch also corrects the wrong usage of x32-avx-mpx-avx512, which is
> replaced by x32-avx-avx512. The MPX feature is not implemented in x32 mode.
>
> gdb/Changelog:
> 2016-04-18 Michael Sturm <michael.sturm@intel.com>
>
> * amd64-linux-tdep.c (features/i386/amd64-avx-avx512-linux.c):
> New include.
> (features/i386/x32-avx-mpx-avx512-linux.c): Renamed include.
> (amd64_linux_core_read_description): Add dedicated cases for
> X86_XSTATE_AVX_AVX512_MASK and return appropriate tdesc.
> (_initialize_amd64_linux_tdep): Add calls to
> initialize_tdesc_amd64_avx_avx512_linux () and
> initialize_tdesc_x32_avx_avx512_linux ().
Don't add " ()" when you mention names of functions. Here and
throughout (the series).
> * amd64-linux.tdep.h (tdesc_amd64_avx_avx512_linux): New prototype.
> (tdesc_x32_avx_mpx_avx512_linux): Renamed prototype.
Is that old or new name? Renames should say both. E.g., this
is a frequently used style:
(foo): Rename to ...
(bar): ... this.
> * features/i386/x32-avx-mpx-avx512-linux.c: Regenerated from renamed XML
> file.
This is incorrect -- it must be that x32-avx-mpx-avx512-linux.c was deleted,
and features/i386/x32-avx-avx512-linux.c is a new file.
> * features/i386/x32-avx-mpx-avx512-linux.xml: Renamed to
> features/i386/x32-avx-avx512-linux.xml
Missing period.
> * Makefile.in (clean): Added handling new source files
^^ spurious double space.
"handling of" ? Or just say "Remove i386-avx-avx512.c, ..."
> i386-avx-avx512.c, i386-avx-avx512-linux.c, amd64-avx-avx512.c,
> amd64-avx-avx512-linux.c.
> (initialize_low_tracepoint): Added init_registers_amd64_avx_avx512_linux.
> * linux-i386-ipa.c (get_ipa_tdesc): Added dedicated case for
> X86_TDESC_AVX_AVX512 and returne appropriate tdesc.
typo "returne".
The comments above apply to multiple places, but I just pointed out one
of each.
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index d70c4b0..7e54a8f 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -44,11 +44,12 @@
> #include "features/i386/amd64-avx-linux.c"
> #include "features/i386/amd64-mpx-linux.c"
> #include "features/i386/amd64-avx-mpx-linux.c"
> +#include "features/i386/amd64-avx-avx512-linux.c"
> #include "features/i386/amd64-avx-mpx-avx512-linux.c"
>
> #include "features/i386/x32-linux.c"
> #include "features/i386/x32-avx-linux.c"
> -#include "features/i386/x32-avx-mpx-avx512-linux.c"
> +#include "features/i386/x32-avx-avx512-linux.c"
>
> /* The syscall's XML filename for i386. */
> #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml"
> @@ -1581,11 +1582,16 @@ amd64_linux_core_read_description (struct gdbarch *gdbarch,
> switch (xcr0 & X86_XSTATE_ALL_MASK)
> {
> case X86_XSTATE_AVX_MPX_AVX512_MASK:
> - case X86_XSTATE_AVX_AVX512_MASK:
> if (gdbarch_ptr_bit (gdbarch) == 32)
> - return tdesc_x32_avx_mpx_avx512_linux;
> + /* No x32 MPX falling back to AVX-AVX512. */
I know this string appears in other places already, but
I'd like to point out that it sounds quite odd to me.
I suggest saying what appears on the gdbserver hunk:
/* No MPX on x32, fallback to AVX-AVX512. */
> diff --git a/gdb/features/i386/x32-avx-avx512-linux.xml b/gdb/features/i386/x32-avx-avx512-linux.xml
> new file mode 100644
> index 0000000..36bcbb7
> --- /dev/null
> +++ b/gdb/features/i386/x32-avx-avx512-linux.xml
> @@ -0,0 +1,19 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2014-2016 Free Software Foundation, Inc.
> +
> + Copying and distribution of this file, with or without modification,
> + are permitted in any medium without royalty provided the copyright
> + notice and this notice are preserved. -->
> +
> +<!-- X32 with AVX, MPX, AVX512 - Includes Linux-only special "register". -->
MPX shouldn't be here, I believe.
> diff --git a/gdb/features/i386/x32-avx-avx512.xml b/gdb/features/i386/x32-avx-avx512.xml
> new file mode 100644
> index 0000000..9e17826
> --- /dev/null
> +++ b/gdb/features/i386/x32-avx-avx512.xml
> @@ -0,0 +1,17 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2014-2016 Free Software Foundation, Inc.
> +
> + Copying and distribution of this file, with or without modification,
> + are permitted in any medium without royalty provided the copyright
> + notice and this notice are preserved. -->
> +
> +<!-- X32 with AVX, MPX, AVX512 -->
Ditto.
Thanks,
Pedro Alves