Re: [RFC][PATCH 1/2] RISC-V: Add cgen cpu description and generated files

[Jim Wilson <jimw at sifive dot com>]

On Thu, Sep 12, 2019 at 4:42 AM Ed Jones <ed.jones@embecosm.com> wrote:
> This adds a cpu definition for RISC-V as well as the cgen-generated
> opcodes files. This does not replace the existing opcodes files and it
> has no effect on the RISC-V binutils at this time except to include
> cgen dependencies and generated files in the libopcodes build. These
> files lay the foundation for a RISC-V cgen-based simulator.

I'm wondering how maintenance will work.  When we make changes to the
existing assembler/disassembler, how does the cgen support get updated?  We
probably need to specify a maintainer for this RISC-V cgen code.  Presumably
Edward Jones or someone else at Embecosm.

+;  -ne0-ne2 : Any register exception 'zero' or 'sp'

exception->except

+  (machs rv32i rv32ic rv32im rv32imc rv32g rv32gc rv32gqc
+         rv64i rv64ic rv64im rv64imc rv64g rv64gc rv64gqc)

It would be nice if the --with-arch configure option could be used to set
the default mach.  Currently binutils doesn't use --with-arch though, only
gcc does.  This would effectively require maintainer mode because this
would require regenerating the cgen output files.  This could be a future
project, not required for acceptance.

+(define-pmacro i-ext (RVEXT RV32I RV64I))
+(define-pmacro a-ext (RVEXT RV32A RV64A))

This is missing E support, but not a critical problem for the simulator.  It
would be critical for the assembler.  Looks like this isn't fully implemented.

+(define-pmacro rv32m-machs (MACH rv32im rv32imc rv32g rv32gc rv32gqc))
+(define-pmacro rv64m-machs (MACH rv64im rv64imc rv64g rv64gc rv64gqc))
+(define-pmacro m-machs     (MACH rv32im rv32imc rv32g rv32gc rv32gqc
+                                 rv64im rv64imc rv64g rv64gc rv64gqc))

This looks inconvenient.  Everytime we add an architecture combination
we need to update all of these macros.  But maybe we can put them in an
include file, and then write a program to generate that file.

+  (name h-gpr)
...
+  (type register WI (64))

Not clear why you are defining 64 gprs.  There are only 32.

+  (name h-gpr-not-sp)
+  (name h-zero)

These aren't ignoring writes to reg 0 like h-gpr does.  But I suppose that is
maybe because they are never used for destination operands, only source
operands, so it doesn't matter.  Might be confusing if this changes later, but
not a critical problem.

Something to mark stuff that is incomplete would be nice.  For instance
c.slli64 are implemented as hints which is correct for rv32c/64c but not 128c,
and this isn't clearly marked.  The rv32fc instructions are defined as rv32c
specific which is wrong.  There is a comment that mentions this, but no easy
way to find it.  So maybe TODO or ??? markers for stuff that is incomplete.
This incidentally means an invalid rv32fc instruction executed on a rv32ic
target won't get an illegal instruction trap, and also means that the assembler
will accept an instruction when it shouldn't.  There is a similar problem with
the rv32dc/64dc instructions.  There seems to be a general problem here with
describing things that require the AND of multiple extensions, or the OR of
multiple extensions.  I found a FIXME farther down which is OK too.  I'd just
suggest that this should be used in more places.  I also found a "not supported
yet" farther down.

"don't to keep" -> "don't have to keep"

Defines fence.tso and fence.i as part of the base ISA, but they are Ztso and
Zifencei extensions.  This is something the existing tools get wrong too,
though we are going to have to fix it soon.

The rv32fc problem repeats in the macro section with p-c-flwsp.  And again
with the rv64fd set of compressed instruction macros.

The disassembler isn't setting insn_info_valid and the other related
fields.  The current disassembler does.

Overall this looks good, but I'm concerned that the lack of E support, and
the difficulty of specifying all of the architecture variants properly means
it won't work well for the assembler.

Jim