From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11023 invoked by alias); 28 Mar 2010 14:25:01 -0000 Received: (qmail 11010 invoked by uid 22791); 28 Mar 2010 14:25:00 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40 X-Spam-Check-By: sourceware.org Received: from mail-vw0-f41.google.com (HELO mail-vw0-f41.google.com) (209.85.212.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 28 Mar 2010 14:24:55 +0000 Received: by vws20 with SMTP id 20so5343089vws.0 for ; Sun, 28 Mar 2010 07:24:53 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.90.201 with HTTP; Sun, 28 Mar 2010 07:24:53 -0700 (PDT) In-Reply-To: <201003281155.o2SBtYEk029587@glazunov.sibelius.xs4all.nl> References: <20100304180219.GA10826@intel.com> <20100304180408.GA10869@intel.com> <20100304180643.GB10869@intel.com> <20100306222037.GD21133@intel.com> <201003271547.o2RFlvNI002277@glazunov.sibelius.xs4all.nl> <6dc9ffc81003271837l761ac147t7ba88988cb99fcb0@mail.gmail.com> <201003281155.o2SBtYEk029587@glazunov.sibelius.xs4all.nl> Date: Sun, 28 Mar 2010 14:25:00 -0000 Received: by 10.220.107.17 with SMTP id z17mr2148081vco.75.1269786293166; Sun, 28 Mar 2010 07:24:53 -0700 (PDT) Message-ID: <6dc9ffc81003280724o46b2fd80v6797a7ac765af333@mail.gmail.com> Subject: Re: PATCH: 3/6 [2nd try]: Add AVX support (i386 changes) From: "H.J. Lu" To: Mark Kettenis Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-03/txt/msg00941.txt.bz2 On Sun, Mar 28, 2010 at 4:55 AM, Mark Kettenis wr= ote: >> Date: Sat, 27 Mar 2010 18:37:41 -0700 >> From: "H.J. Lu" >> >> >> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c >> >> index b23c109..66ecf84 100644 >> >> --- a/gdb/i386-linux-tdep.c >> >> +++ b/gdb/i386-linux-tdep.c >> >> +#include "i387-tdep.h" >> >> +#include "i386-xstate.h" >> >> + >> >> =A0/* The syscall's XML filename for i386. =A0*/ >> >> =A0#define XML_SYSCALL_FILENAME_I386 "syscalls/i386-linux.xml" >> >> >> >> @@ -47,13 +50,15 @@ >> >> =A0#include >> >> >> >> =A0#include "features/i386/i386-linux.c" >> >> +#include "features/i386/i386-avx-linux.c" >> >> >> >> =A0/* Supported register note sections. =A0*/ >> >> -static struct core_regset_section i386_linux_regset_sections[] =3D >> >> +struct core_regset_section i386_linux_regset_sections[] =3D >> > >> > Why do you make this non-static? >> >> I need to change size of .reg-xstate section from i386-linux-nat.c. > > But then, why do you have the i386_linux_update_xstateregs() function > if you still need to pass the array itself around? i386-linux-nat.c calls i386_linux_update_xstateregs with i386_linux_regset_sections. Also amd64-linux-nat.c calls i386_linux_update_xstateregset with amd64_linux_regset_sections. If I don't make amd64_linux_regset_sections and i386_linux_regset_sections global, I have to write i386_linux_update_xstateregset and amd64_linux_update_xstateregset. The only difference of 2 functions will be amd64_linux_regset_sections vs. i386_linux_regset_sections. > Anyway, how about setting the size of the .reg-xstate to > I386_XSTATE_SSE_SIZE unconditionally? =A0Tools will look at xcr0 value > encoded in there to determine what information in there is valid, so > dumping a little bit more than strictly necessary shouldn't be a > problem. That will make the code more complex since the generic gcore implementation will have to adjust section size based on XCR0. But if it is what is required, I will make the change. > It would simplify things a bit. =A0Less code is good! > > >> >> + =A0 =A0 =A0/* Check extended state size. =A0*/ >> >> + =A0 =A0 =A0if (size < I386_XSTATE_AVX_SIZE) >> >> + =A0 =A0 xcr0 =3D I386_XSTATE_SSE_MASK; >> >> + =A0 =A0 =A0else >> >> + =A0 =A0 { >> >> + =A0 =A0 =A0 char contents[8]; >> >> + >> >> + =A0 =A0 =A0 if (! bfd_get_section_contents (abfd, xstate, contents, >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 (file_ptr) I386_LINUX_XSAVE_XCR0_OFFSET, >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 8)) >> > >> > Is that cast really necessary? >> >> I just follow the tradition. Most of bfd_get_section_contents calls have >> (file_ptr) cast. It may be used to avoid 32bit vs 64bit VMA warning. > > Please don't use casts when they're not absolutely necessary; they > tend to hide bugs. I will make the change. >> >> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c >> >> index 05afa56..8ced34a 100644 >> >> --- a/gdb/i386-tdep.c >> >> +++ b/gdb/i386-tdep.c >> >> @@ -2183,6 +2241,59 @@ i387_ext_type (struct gdbarch *gdbarch) >> >> =A0 =A0return tdep->i387_ext_type; >> >> =A0} >> >> >> >> +/* Construct vector type for pseudo XMM registers. =A0We can't use >> >> + =A0 tdesc_find_type since XMM isn't described in target description= . =A0*/ >> > >> > I'm confused here. =A0If you have a non-AVX target, why do you need a = 256-bit vector type? >> >> i386_ymm_type is only called from >> >> =A0 else if (i386_ymm_regnum_p (gdbarch, regnum)) >> =A0 =A0 return i386_ymm_type (gdbarch); >> >> It won't be called if you have a non-AVX target. > > Sorry; that confuses me even more. =A0Let me try to explain again what > puzzles me. =A0The pseudo XMM registers are 128-bit, so why are you > building a 256-bit type? =A0Is the problem simply that the comment is > wrong and you're talking about pseudo YMM registers here? Ooops. I meant "pseudo YMM registers". I will update comments. >> >> @@ -5854,9 +6071,13 @@ i386_gdbarch_init (struct gdbarch_info info, s= truct gdbarch_list *arches) >> >> =A0 =A0set_tdesc_pseudo_register_type (gdbarch, i386_pseudo_register_= type); >> >> =A0 =A0set_tdesc_pseudo_register_name (gdbarch, i386_pseudo_register_= name); >> >> >> >> - =A0/* The default ABI includes general-purpose registers, >> >> - =A0 =A0 floating-point registers, and the SSE registers. =A0*/ >> >> - =A0set_gdbarch_num_regs (gdbarch, I386_SSE_NUM_REGS); >> >> + =A0/* Override the normal target description method to make the AVX >> >> + =A0 =A0 upper halves anonymous. =A0*/ >> >> + =A0set_gdbarch_register_name (gdbarch, i386_register_name); >> >> + >> >> + =A0/* The default ABI includes general-purpose registers, floating-= point >> >> + =A0 =A0 registers, the SSE registers and the upper AVX registers. = =A0*/ >> >> + =A0set_gdbarch_num_regs (gdbarch, I386_AVX_NUM_REGS); >> > >> > Isn't it better to leave the AVX registers out of the default target, >> > and only provide them if we're talking to a target (native or remote) >> > that indicates it supports them? >> >> That is set =A0to a value higher enough to support AVX. The actual number >> of registers will be set properly later. See: > > OK, then please adjust the comment to say something like: > > =A0 =A0/* Even though the default ABI only includes general-purpose regis= ters, > =A0 =A0 =A0 floating-point registers and the SSE registers, we have to le= ave a > =A0 =A0 =A0 gap for the upper AVX registers. =A0*/ > I will make the change. Thanks. --=20 H.J.