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

[cgd at broadcom dot com]

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<RD>, r<RS>"
>   *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