[PATCH] RISC-V: Add missing c.unimp instruction.

[Jim Wilson]

On Thu, Nov 29, 2018 at 5:46 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> >  {"unimp",       0, {"I", 0},   "",  MATCH_CSRRW | (CSR_CYCLE << OP_SH_CSR), 0xffffffffU,  match_opcode, 0 }, /* csrw cycle, x0 */
> I think this is an alias: the ISA manual doesn't actually define a single
> canonical unimplemented instruction, this is just an instruction that is not
> listed in the ISA spec and is unlikely to ever be implemented.

FYI The alias support is horribly broken.  If you actually want this
to work, then someone has to go through and fix all of the broken
patterns.  This is on my list of things to do, but not on my short
list, as I don't think it is important enough.

Yes, this is technically an alias, as cycle is a read-only csr
register, and hence a write to cycle must always trap.  It must also
trap if cycle is not implemented, or if csrrw is not implemented.
Hence it always traps.  But I think decoding it as the actual csrrw
cycle instruction could be confusing.  This isn't the normal case
where we are using a lt for a ge for instance, but in this case we are
using csrrw as an illegal instruction and not as a csrrw instruction.
By the way, the RISC-V assembler manual was updated today to mention
that this is the 32-bit unimp instruction.

> >  {"ebreak",      0, {"C", 0},   "",  MATCH_C_EBREAK, MASK_C_EBREAK, match_opcode, INSN_ALIAS },
> >  {"ebreak",      0, {"I", 0},   "",    MATCH_EBREAK, MASK_EBREAK, match_opcode, 0 },
> > @@ -696,6 +696,7 @@ const struct riscv_opcode riscv_opcodes[] =
> >  {"fcvt.q.lu", 64, {"Q", 0}, "D,s,m",  MATCH_FCVT_Q_LU, MASK_FCVT_Q_LU, match_opcode, 0 },
> >
> >  /* Compressed instructions.  */
> > +{"c.unimp",    0, {"C", 0},   "",  0, 0xffffU,  match_opcode, 0 },
>
> I think this is also an alias, largely for the same reason.

But if you mark it as an alias, then the assembler won't be able to
decode it, as there is no other instruction that can match.  It makes
more sense to leave it there.  This is also documented as the 16-bit
unimp instruction in the RISC-V assembler manual, added today.

> >  {"c.ebreak",   0, {"C", 0},   "",  MATCH_C_EBREAK, MASK_C_EBREAK, match_opcode, 0 },
> >  {"c.jr",       0, {"C", 0},   "d",  MATCH_C_JR, MASK_C_JR, match_rd_nonzero, INSN_BRANCH },
> >  {"c.jalr",     0, {"C", 0},   "d",  MATCH_C_JALR, MASK_C_JALR, match_rd_nonzero, INSN_JSR },
>
> This one is explicitly listed in the ISA manual as an illegal instruction, so
> it's correct to not be an alias.

c.jalr isn't an illegal instruction.  Maybe you meant to type this on
another line?

Jim