This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Jim,

Thank you for taking your time to review these patches! I've added my
comments inline.

On Wed, 25 Sep 2019 at 21:08, Jim Wilson <jimw@sifive.com> wrote:
>
> 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.

I expect that we at Embecosm would maintain the changes relevant to
the assembler/disassembler out of tree on the Embecosm github, until
we could demonstrate that it has identical behaviour to the existing
assembler/disassembler.

As it stands the patches I have submitted also contain the assembly
description which aren't really relevant to the simulator. Obviously
these will diverge from the behavior of the existing tools unless they
are maintained, so it's probably an unnecessary burden to put those
changes in tree too. I'll see if I can strip out all of these parts of
the cpu description (and the generated opcodes files) for the next
version of the patch.

I assume that I will be the maintainer for this code, but there are
others at Embecosm who would be able to take over this role too.

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

Noted! I'll incorporate all of these small fixes in the next iteration
of this patch.

> +  (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.

For the unsubmitted changes to binutils I believe I made the
assembler/disassembler default to enabling all of the extensions, but
it would definitely be worthwhile making it configurable.

For the simulator rv<xlen>gc is chosen when the simulator loads a
riscv32/riscv64 bfd. It's possible for the user to override the model
to one of the above machs, but it would also be nice if that had a
different default. I'll look into this, but I doubt I'll incorporate
it into these patches.

> +(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.

Yes, I'll add this to a list of incomplete features.

> +(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.

Yes, it's horrible - I couldn't figure out a way to do this within the
cgen description itself so it might have to be done in a preprocessing
step as you suggest.

> +  (name h-gpr)
> ...
> +  (type register WI (64))
>
> Not clear why you are defining 64 gprs.  There are only 32.

Noted!

> +  (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.

Those two sets of registers should both defer to the h-gpr setter,
from the following lines:

  (define hardware
    (name h-gpr-not-sp)
    ...
    (set (index newval) (set (reg h-gpr index) newval))
  )

This should mean that they use the "set" code for h-gpr. The "h-gpr"
setter itself uses (raw-reg h-gpr index) instead of (reg ...) which
indexes the underlying array which backs the registers.

Nonetheless I guess I need to add a comment to explain the above.

> 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.

Okay. I'll attempt to list out the incomplete parts at the top, along
with some consistent markers and comments in the code itself.

And I'll add the "hints" and "rv32fc" problems to that list.

> 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.

Noted! I think the base I instructions were implemented before fence
was separated out into its own extension.

> 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.

Okay, it seems there quite a lot more missing on the assembler side
than the simulator side. Is E primarily important for the assembler?
Do you think it would be okay to exclude for now and add in at a later
point?

Thanks,
Ed


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]