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

[cgd@broadcom.com]

At 09 Oct 2002 15:14:43 +0100, Richard Sandiford wrote:
> Thanks for the review.

That's what I'm here for.  8-)  (Sorry for taking so long to get to
this, but, as i mentioned in private mail before I left, I was out of
the office.  8-)


> Good catch.  I checked that vr5000 implied mipsIV, but not the
> other way around (duh!).

(I dunno whether you were using the horrible compare_igen_models
script or not, but it does both ways for you and is ... much easier
than hand inspection.  8-)


> Yeah.  I don't understand why, really, since the vr41xx processors don't
> have an FPU.  Maybe the target environment was assumed to provide an
> emulator or something?  But key instructions (like bc1f and bc1t ;-)
> are missing, so very little FP code is going to work.

I don't know, the code predates my time here.  8-)

FWIW, my personal preference is to have a simulator which is good
enough to act like real hardware, then, if necessary and appropriate
(i.e., trying to simulate a firmware or other SW environment which has
an emulator) do actual emulation.

(One implication of this is that a 'good' emulation for a particular
processor might well trap on certain FP cases unimplemented in HW.)

There are both plusses and minuses to this approach, of course.


> > 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.
> 
> OK.  Guilt at my earlier lameness forced me to give this a go.
> New version attached.  Not pretty (and I didn't use awk ;-) but
> it should reduce the amount of cut&paste if other targets use
> MULTI.  I've also removed the need to define sim_multi_filter
> and sim_multi_machine, since igen can cope with the same machine
> & filter being given twice.

Heh.  (no awk?!  what will we do?!)


> > 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.
> 
> Where do you think this should happen?  In sim_engine_run()?

To be honest, I'm not really sure.  However, I do believe that that is
too late.  8-)

(The problem is that simulator machine state initialization has
happened by the time you invoke sim_engine_run(), and that may have to
differe depending on the type of machine you're simulating.)

Ideally what you want is a replacement for

	STATE_ARCHITECTURE (sd) ? STATE_ARCHITECTURE (sd)->mach : 0

that does the correct default handling, but is very very fast.  Alas,
I don't know that that's possible.  8-)

Probably the right thing is to just turn it into a function call if
doing MULTI, else have it #defined to the correct specific value if
not MULTI (i.e., a single-arch sim).

People running the GCC test suite in a MULTI sim don't care if they're
getting 200k insns/sec or 1m insns/sec...  8-)

On the other hand, people who want a really fast sim will build one
that targets a single CPU, which will get the #define which also
allows additional optimizations (code to be elided).  (Having been in
the position of booting firmware under a sim, then booting OSes under
said firmware, I can say that I like the fastest sim possible
sometimes.  8-)

Thoughts?


> + # The MULTI generator can combine several simulation engines into
> + # one executable.  A configuration which uses the MULTI should
> + # set ${sim_multi_arch_configs} to the list of engines to build.
> + # Each space-separated entry has the form NAME:MACHINE:FILTER@ARCHS,
> + # where:
> + #
> + # - NAME is a C-compatible prefix for the engine,
> + # - MACHINE is a -M argument,
> + # - FILTER is a -F argument, and
> + # - ARCHS is a comma-separated list of bfd machines that the
> + #     simulator can run.
> + #
> + # Each entry will have a separate simulation engine whose prefix is
> + # m32<NAME>.  If the machine list includes "mips16", there will also
> + # be a mips16 engine, prefix m16<NAME>.  The mips16 engine will be
> + # generated using the same machine list as the 32-bit version,
> + # but the filter will be "16" instead of FILTER.
> + #
> + # The simulator compares the bfd mach against ARCHS to decide
> + # which engine to use.  Entries in ARCHS can be a bfd_mach_mips*
> + # value with "bfd_mach_mips" removed, or the special string
> + # "default" to indicate that a particular engine should be
> + # the default.

I believe there _must_ be one default entry, and this should be
checked for in the code too.  What do you think?

Why use "@" rather than another :... makes the seds slightly easier,
that it?

Suggest either:

* archs have only bfd_mach_ prepended i.e., mips4111, mips_sb1, etc.
  The current method is ... not so pretty for archs whose bfd define
  isn't a number.  This makes the specifictions a bit longer, but IMO
  that's not biggie.

* more special casing, e.g.:

	[0-9]* add bfd_mach_mips
	[a-z]* add bfd_mach_mips_

  8-)  This could get out of hand, though.


(personally, my feeling is that adding even more defines of the form
bfd_mach_mipsNNNN is ... not the right thing.  It should be
bfd_mach_mips_vendnum
	      ^^^^^^^ vendor letter/number combo, e.g. sb1, vrNNNN
         ^^^^ ARCHITECTURE!!!
or whatever...  but that's water under the bridge.)



> + if test ${sim_gen} = MULTI; then
> +   rm -f multi-include.h multi-switch.c
> +   sim_multi_flags=
> +   sim_multi_obj=multi_run.o
> +   for fc in ${sim_multi_arch_configs}; do
> +     c=`echo ${fc} | sed 's/@.*//'`
> +     archs=`echo ${fc} | sed 's/.*@//'`
> +     name=`echo ${c} | sed 's/:.*//'`
> +     m=`echo ${c} | sed 's/.*:\(.*\):.*/\1/'`
> +     f=`echo ${c} | sed 's/.*://'`
> +     sim_multi_flags="${sim_multi_flags} -F ${f} -M ${m}"
> +     case $c in
> +       *:*mips16*:*)
> + 	ws="m32 m16"
> + 	sim_multi_src="${sim_multi_src} m16${name}_run.c"
> + 	sim_multi_obj="${sim_multi_obj} m16${name}_run.o"
> +         sim_multi_flags="${sim_multi_flags} -F 16"
> + 	;;
> +       *)
> + 	ws=m32
> + 	;;
> +     esac
> +     for w in ${ws}; do
> +       for base in engine icache idecode model semantics support; do
> + 	sim_multi_src="${sim_multi_src} ${w}${name}_${base}.c"
> + 	sim_multi_src="${sim_multi_src} ${w}${name}_${base}.h"
> + 	sim_multi_obj="${sim_multi_obj} ${w}${name}_${base}.o"
> +       done
> +       sim_multi_configs="${sim_multi_configs} ${w}${c}"
> +     done
> +     echo "#include \"${w}${name}_engine.h\"" >> multi-include.h
> +     for a in `echo $archs | sed 's/,/ /g'`; do
> +       case $a in
> +         default) echo "default:" >> multi-switch.c ;;
> +         *) echo "case bfd_mach_mips$a:" >> multi-switch.c ;;
> +       esac
> +     done
> +     echo "  ${w}${name}_engine_run (sd, next_cpu_nr, nr_cpus, signal);" \
> +       >> multi-switch.c
> +     echo "  break;" >> multi-switch.c
> +   done
> + else
> +   # For clean-extra
> +   sim_multi_src=doesnt-exist.c
> + fi
>   sim_igen_flags="-F ${sim_igen_filter} ${sim_igen_machine} ${sim_igen_smp}"
>   sim_m16_flags=" -F ${sim_m16_filter}  ${sim_m16_machine}  ${sim_igen_smp}"
>   AC_SUBST(sim_igen_flags)
>   AC_SUBST(sim_m16_flags)
>   AC_SUBST(sim_gen)
> + AC_SUBST(sim_multi_flags)
> + AC_SUBST(sim_multi_configs)
> + AC_SUBST(sim_multi_src)
> + AC_SUBST(sim_multi_obj)

err, shouldn't multi-switch.c be cleaned somehow (during distclean, i
guess, since it's config-generated).

Also, shouldn't multi-run.o depend on multi-switch.c so that the right
thing happens after re-config?

(obviously, if there's a fn to pick the right machine type, the case
statement in the engine generated above doesn't need to include the
default, that fn does.)



chris