Re: sim/mips patch: add support for more NEC VR targets

cgd
2002-11-27T01:48:56Z

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