From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14450 invoked by alias); 27 Nov 2002 01:48:56 -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 14443 invoked from network); 27 Nov 2002 01:48:55 -0000 Received: from unknown (HELO mms2.broadcom.com) (63.70.210.59) by sources.redhat.com with SMTP; 27 Nov 2002 01:48:55 -0000 Received: from 63.70.210.1 by mms2.broadcom.com with ESMTP (Broadcom MMS2 SMTP Relay (MMS v5.5.0)); Tue, 26 Nov 2002 17:46:20 -0700 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 RAA16704; Tue, 26 Nov 2002 17:48:50 -0800 (PST) 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 gAR1mnER025111; Tue, 26 Nov 2002 17:48:49 -0800 (PST) Received: (from cgd@localhost) by dt-sj3-118.sj.broadcom.com ( 8.9.1/SJ8.9.1) id RAA03706; Tue, 26 Nov 2002 17:48:45 -0800 (PST) To: ac131313@redhat.com cc: "Richard Sandiford" , gdb-patches@sources.redhat.com Subject: Re: sim/mips patch: add support for more NEC VR targets References: <3DE417F8.8030209@redhat.com> From: cgd@broadcom.com Date: Tue, 26 Nov 2002 17:48:00 -0000 In-Reply-To: ac131313@redhat.com's message of "Wed, 27 Nov 2002 00:55:43 +0000 (UTC)" Message-ID: MIME-Version: 1.0 X-WSS-ID: 11FAFC666555-01-01 Content-Type: text/plain Content-Transfer-Encoding: 7bit X-SW-Source: 2002-11/txt/msg00670.txt.bz2 At Wed, 27 Nov 2002 00:55:43 +0000 (UTC), "Andrew Cagney" wrote: > > :function:::int:check_mf_cycles:hilo_history *history, signed64 time, const char *new > > { > > + /* There are no timing requirements in vr5500 code. */ > > + if (MIPS_MACH (SD) == bfd_mach_mips5500) > > + return 1; > > if (history->mf.timestamp + 3 > time) > > { > > sim_engine_abort (SD, CPU, CIA, "HILO: %s: %s at 0x%08lx too close to MF at 0x%08lx\n", > > Just this, > > Add: > > function:::int:check_mf_cycles:hilo_history *history, signed64 time, > const char *new > *vr5500 > { > return 1; > } > > and then tag the old function with the other CPU variants. So, if you do this, then the old function will be tagged... well, mipsIV at least, right? And then given the way this patch invokes igen for multi-arch sims, AFAIK _both_ this check_mf_cycles and the 'other' check_mf_cycles will be included. So, if you want to go this route, really, you have to add vr5500 to every bloody mipsIV instruction. > that way > if (MIPS_MACH (SD) == bfd_mach_mips5500) > is not needed and the compiler can (if configured with sufficient > inlining) eliminate the entire function call. FWIW, if configured with sufficient inlining and for a single arch, even with the check it could do that i believe. Like i said before, I don't think it's that important to squeeze the last bit of speed out, for people building multi-arch sims. > This post: > > http://sources.redhat.com/ml/gdb-patches/2002-11/msg00512.html > > We have had very bad experiences with trying to make a single function > > serve two different ABI's in the past. (mips_push_arguments seems to > > have been cleaned up since I last looked; it was a real mess.) So > > while using things like 'REGISTER_SIZE' and > > 'S390_STACK_PARAMETER_ALIGNMENT' are clearly a good idea, for the sake > > of the other stuff I'd like to see a separate 's390x_push_arguments' > > function written that does things right for the s390x's ABI. The > > helper functions like `is_simple_arg' should be duplicated, rather > > than testing GDB_TARGET_IS_ESAME. > > Gives the thrust of the rationale. Namely, its better to, from the > start, have separate independant functions and not confuse things by > adding more and more if(ISA/ABI) specific gunk. As JimB noted, the MIPS > is the not so shining example of how to do things (i.e., how things can > go wrong). Looked at another way, this means that you replace a single explicit "if (ISA)" check in this case with ... dozens or hundreds of implicit checks (the implied check being inclusion or not, for the given architecture). If what you really have is common code with 3 exceptions, i'd rather see three exceptions. > For the sim, the original gencode tried to be smart and combine isa > variants. Given the age of the code, the number of CPU variants, and > the number of developers making changes (the two are comparable and both > are large!), it ended to end up with a total mess. Someone trying to > add one ISA would [always, I'm pretty sure it was] broke the other ISAs > (only discovered months later) by either also adding instructions to > other ISAs or, worse, removing/changing existing instructions. Ulgh! > > IGEN and MIPS.IGEN gave up. I agree that it's a potential problem, but to my mind there are two solutions to the problem, one of which has to be applied to get "quick, reliable" updates to the simulator: * never, ever change the existing code. (The possible exception here is "obvious" fixes, but, well, if you don't test them then you can still screw them up.) * more, and more complete testing, to find the brokenness. What you're advocating, to my mind, boils down the former: duplicate code then modify it, use the duplicated code or if you want to duplicate again and modify again, etc. I think that will lead to monstroustly bad maintainability. (BTW, I don't actually think that the ABI comparison is a good one.) > Instead, the developer gets to spend a few minutes adding an ISA > selector to every relevant instruction. That's in the noise when > compared to the amount of time that needs to be spent auding the > instruction set looking for where the vendor has an instruction variant > that deviates from the official spec (how many mul/adds are there?). In > the case of a variant, the instruction can cloned, taged and changed > with the absolute certaintity that it won't break any other instruction, > because none of the other instructions / ISAs are affected. Where "relevant" is like virtual: just delete it from the sentence, and you keep the same meaning. 8-) I agree with your argument for "standard ISA with instructions removed" processors. However, i don't see that there's ANY reason that if 17 different processors add madd to mipsIII, each mipsIII instruction should have to be tagged with all of those 17 different ISA names. Certainly, I wouldn't want to be working in the resulting code base. > So, while this might sux from an asthetic point of view, it definitly > doesn't sux from the point of view of being able to confidently, quickly > and reliably update the simulator. I think the only way you can have that is to build it and test it and build it more and test it more. Otherwise, i can't see how you're not afraid of making any change to existing code. I'm willing to break some eggs to make an omelete, here; i've got substantial work that i'd still like to see in the mips sim, and i'm quite sure that in getting it in i'll break (more 8-) things. If the goal here is to absolutely minimize potential code breakage, and as a result of that a decision is made either to not substantially enhance the code, or to produce IMO "nasty" unmaintainable code... that doesn't seem like a positive thing, to me. > (cgd, does this answer your question?) Not really. 8-) First of all i'm not sure which question you were trying to answer, even (today's, i.e. "veto, anyone?" or a previous question). Is your intent here to say that really, "the mips sim will/must be done the way you outline above," or is it to provide a rationale for why you think it should be done that way. (to my mind, it could be either; you're a mips sim maintainer long before i got here.) Based on what you've said above, I still don't agree that your suggested way of doing this is the right one. chris