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

[Palmer Dabbelt]

On Thu, 29 Nov 2018 18:40:12 PST (-0800), Jim Wilson wrote:
> 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.

I agree that auditing the list isn't high priority, but that doesn't mean we 
should skip fixing the bugs that jump out.  I can send the patch if you want.

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

I know, I merged it :).  That's how I knew it was an alias so quickly, it's 
listed under the alias section

>
>> >  {"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?

Ya, sorry, I meant this

>> {"c.ebreak",   0, {"C", 0},   "",  MATCH_C_EBREAK, MASK_C_EBREAK, match_opcode, 0 },
>
> This one is explicitly listed in the ISA manual as an illegal instruction, so
> it's correct to not be an alias.