From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25854 invoked by alias); 4 Oct 2002 17:54:54 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 25844 invoked from network); 4 Oct 2002 17:54:53 -0000 Received: from unknown (HELO mms3.broadcom.com) (63.70.210.38) by sources.redhat.com with SMTP; 4 Oct 2002 17:54:53 -0000 Received: from 63.70.210.1 by mms3.broadcom.com with ESMTP (Broadcom MMS-3 SMTP Relay (MMS v4.7);); Fri, 04 Oct 2002 10:54:53 -0700 X-Server-Uuid: 1e1caf3a-b686-11d4-a6a3-00508bfc9ae5 Received: from mail-sj1-5.sj.broadcom.com (mail-sj1-5.sj.broadcom.com [10.16.128.236]) by mon-irva-11.broadcom.com (8.9.1/8.9.1) with ESMTP id KAA12791; Fri, 4 Oct 2002 10:54:53 -0700 (PDT) Received: from dt-sj3-118.sj.broadcom.com (dt-sj3-118 [10.21.64.118]) by mail-sj1-5.sj.broadcom.com (8.12.4/8.12.4/SSF) with ESMTP id g94HpPER010894; Fri, 4 Oct 2002 10:51:25 -0700 (PDT) Received: (from cgd@localhost) by dt-sj3-118.sj.broadcom.com ( 8.9.1/SJ8.9.1) id KAA12118; Fri, 4 Oct 2002 10:51:22 -0700 (PDT) To: rsandifo@redhat.com cc: gdb-patches@sources.redhat.com Subject: Re: sim/mips patch: add support for more NEC VR targets References: From: cgd@broadcom.com Date: Fri, 04 Oct 2002 10:54:00 -0000 In-Reply-To: rsandifo@redhat.com's message of "Fri, 4 Oct 2002 16:45:09 +0000 (UTC)" Message-ID: MIME-Version: 1.0 X-WSS-ID: 11830A6720512-01-01 Content-Type: text/plain Content-Transfer-Encoding: 7bit X-SW-Source: 2002-10/txt/msg00133.txt.bz2 At Fri, 4 Oct 2002 16:45:09 +0000 (UTC), "Richard Sandiford" wrote: > The patch below adds vr4120, vr5400 and vr5500 support to > the MIPS simulator. It also handles the mips64vr(el)-elf > configuration (which is already recognised by config.sub). Excellent! I have a few questions/issues, but other than that this should be OK. > - gen-engine.c adds the global prefix to the beginning of > ENGINE_ISSUE_(PREFIX|POSTFIX)_HOOK. It seems MIPS is the > only back-end to define these macros, and it never adds a > prefix. The patch adjusts igen accordingly. Hmm. I don't know igen so well. Andrew? What are your thoughts here? > - The existing vr5000 model selects three-address mult > and dmult instructions, but those instructions aren't > listed in NEC's documentation. There's a three-address > vr5400 mult instruction, but it has a different opcode. > > The vr5000 model only seems to exist for these instructions, > it would otherwise be a standard mipsIV target. Would it > be OK to submit a follow-on patch to remove it? Well, there are a couple of other differences w.r.t: BC0T, DMFC0, DMTC0, COPz... But I don't know whether they're relevant. As far as I'm concerned, if there is no difference, it should probably be removed. Andrew? > - The uses of vr4100 in mips.igen seem to be redundant > with mipsIII. OK to remove them as well? I'm assuming this is a historical thing. In the old world order, every processor type had its own model. In the new world order, which you seem to be adapting nicely to, processors which are "ISA + extensions" use multiple models. Looking at your multi-arch code, you seem to be handling this in the new correct manner. I am concerned w/ one thing here, though: vr4100 selects a bunch of different cop1 instructions than mipsIII does, e.g. CFC1a vs. CFC1b. (the former, used by MIPS3, does cp1 delay slot handling. The latter, used by vr4100, does not.) I don't know which is correct for vr4100. > I'm planning to submit more 4120 patches to FSF GCC soon. In the > meantime, the patch was tested against the NEC GNUPro version. > Please commit if OK. I assume that means that this was tested in the current gdb+sim sources, against that version of GNUpro. There are a couple of specific things that would be nice if you could arrange in follow-on patches, in addition to what you describe above: * it would be good if things like vr_run.c could be automatically generated. It should be easy to do this with a little more info in sim_multi_arch_configs and a little bit of awk. 8-) (I suppose if you don't want to tackle that, it would be OK... but could I sign you up to help test the changes later if you don't do it?) > *************** > *** 229,234 **** > --- 232,240 ---- > > :function:::int:check_mf_cycles:hilo_history *history, signed64 time, const char *new > { > + /* There are no timing requirements in vr5500 code. */ > + if (STATE_ARCHITECTURE (SD)->mach == bfd_mach_mips5500) > + return 1; > if (history->mf.timestamp + 3 > time) > { This is the first case of code like this in the MIPS sim, but it seems like the right thing. > *************** > *** 1010,1015 **** > --- 1016,1022 ---- > "clo r, r" > *mips32: > *mips64: > + *vr5500: (and the related instructions) You might check that the 5500 actually uses the same encodings here, and what it says about unpredicable operation if RT != RD. (this is coded based on the MIPS32/MIPS64 specs, which accomodate an older implementation which used the other reg. than mips32/mips64 did.) Probably right to simulate them w/ the same restrictions, unless vr5500 does something very different. There is an annoying interaction between multi-arching a sim and using STATE_ARCHITECTURE (SD)->mach: e.g. if you have the thing above where you check bfd_mach_mips5500, if mips5500 is the 'default' for your sim (i.e., catches unknown machine types), that check will fail. Really, we need to be sure to check and, if necessary, set 'mach' to a default value when it is set. Other than mentioned above (I don't see any real objections 8-), your patch is OK by me. Andrew might have some comments, and I don't know that I really have the authority to OK changes to the igen bit. chris