Re: [RFC][PATCH 2/2] RISC-V: Add cgen-based simulator port

[Ed Jones <ed dot jones at embecosm dot com>]

Again, thanks for your comments, Jim. I've added my own comments inline.

On Wed, 25 Sep 2019 at 21:16, Jim Wilson <jimw@sifive.com> wrote:
>
> On Thu, Sep 12, 2019 at 4:39 AM Ed Jones <ed.jones@embecosm.com> wrote:
> > This adds a riscv port of the simulator, based on the cgen based cpu
> > description.
>
> I'm not a gdb maintainer, so I can't formally approve the gdb bits.
>
> We will need to specify maintainers for this code.  I'm willing to be a
> secondary maintainer, but presumably someone at Embecosm will want to be
> the primary maintainer, possibly Edward Jones since he submitted the patches.
>
> include/gdb/sim-riscv.h copies a table from gdb/riscv-tdep.h, having two
> copies of this table isn't wise, there should be a single copy that both
> gdb and sim can use, it mentions RiscV, RISC-V is the official spelling

Agreed, I'll look into using a common table. I suspect there are other
places with unofficial spellings of RISC-V, so I'll have a look for
those.

> Looks like the simulator only builds one of 32-bit or 64-bit at a time.  The
> rest of the toolchain supports both at the same time.  This will be
> inconvenient for some use cases.

This is something that I wanted to get working but I think it will be
quite hard to achieve without reworking riscv.cpu or make changes to
cgen itself. The main problem is that cgen will not let you generate a
model for multiple machs if the cpu associated with the machs do not
have the same 'word-bitsize'. So either cgen would need to be modified
to relax that restriction, or we would need to set the 'word-bitsize'
to the larger of the supported xlens and make all of the instruction
semantics depend on the value of xlen at runtime (we do already do
this for a couple of instructions using the 'h-xlen' hardware
element).

> sim/riscv/riscv.c has a function for each supported architecture variant
> which looks inconvenient, as it means we have to specify every single one.

I'll see if there's a better way of doing this. At the very least a
preprocessing macro could be used to stamp out the functions, but I am
wary that adding a new 'mach' already requires updating multiple
files.

> The simulator testcases are really nice.  That is actually a lot more work
> than I would have expected for a simulator testsuite.

Thank you. At the moment we cover a good proportion of the
instructions in the base extensions, and we're working on adding more
tests of the edge-cases.

> The simulator testsuite only works if I configure a rv64gc toolchain.  If
> I configure a rv32i toolchain for instance, then all of the *w, compressed,
> FP, atomic, and mul/div tests fail.  This should be fixable by specifying
> an assembler -march=X option for the minimum arch required by a test, as the
> toolchain supports all arch choices no matter what it is configured for.

Noted.

> When I try to use the simulator inside gdb, for a riscv32-elf target, without
> specifying an executable first, I get
> gamma05:2057$ build-install/bin/riscv32-unknown-elf-gdb -q
> (gdb) target sim
> sim-model.c:210: assertion failed - model != NULL
> (gdb)
> It does work if I start gdb with an executable though.
>
> Otherwise this looks OK to me.

Okay. It looks like the default model is not set up correctly. I'll
file a bug, that should be straightforward to fix.

Thanks,
Ed