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

Discussion:

Jim Wilson

2018-11-29 21:11:45 UTC

We have support for unimp to produce 2 or 4 byte instructions depending on
whether compressed support is on, but we don't have a c.unimp to force use
of the 2 byte instruction form, unlike for all of the other compressed
instructions. This patch adds the missing instruction.

This was tested cross for riscv{32,64}-{elf,linux} with builds and make check
runs. There were no regressions.

Committed.

Jim

opcodes/
* riscv-opc.c (unimp): Mark compressed unimp as INSN_ALIAS.
(c.unimp): New.
---
opcodes/riscv-opc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
index a272e29fee..3da2a7702b 100644
--- a/opcodes/riscv-opc.c
+++ b/opcodes/riscv-opc.c
@@ -198,7 +198,7 @@ match_srxi_as_c_srxi (const struct riscv_opcode *op, insn_t insn)
const struct riscv_opcode riscv_opcodes[] =
{
/* name, xlen, isa, operands, match, mask, match_func, pinfo. */
-{"unimp", 0, {"C", 0}, "", 0, 0xffffU, match_opcode, 0 },
+{"unimp", 0, {"C", 0}, "", 0, 0xffffU, match_opcode, INSN_ALIAS },
{"unimp", 0, {"I", 0}, "", MATCH_CSRRW | (CSR_CYCLE << OP_SH_CSR), 0xffffffffU, match_opcode, 0 }, /* csrw cycle, x0 */
{"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 },
{"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 },

--
2.17.1

Palmer Dabbelt

2018-11-30 01:46:38 UTC

Permalink

Post by Jim Wilson
We have support for unimp to produce 2 or 4 byte instructions depending on
whether compressed support is on, but we don't have a c.unimp to force use
of the 2 byte instruction form, unlike for all of the other compressed
instructions. This patch adds the missing instruction.
This was tested cross for riscv{32,64}-{elf,linux} with builds and make check
runs. There were no regressions.
Committed.
Jim
opcodes/
* riscv-opc.c (unimp): Mark compressed unimp as INSN_ALIAS.
(c.unimp): New.
---
opcodes/riscv-opc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
index a272e29fee..3da2a7702b 100644
--- a/opcodes/riscv-opc.c
+++ b/opcodes/riscv-opc.c
@@ -198,7 +198,7 @@ match_srxi_as_c_srxi (const struct riscv_opcode *op, insn_t insn)
const struct riscv_opcode riscv_opcodes[] =
{
/* name, xlen, isa, operands, match, mask, match_func, pinfo. */
-{"unimp", 0, {"C", 0}, "", 0, 0xffffU, match_opcode, 0 },
+{"unimp", 0, {"C", 0}, "", 0, 0xffffU, match_opcode, INSN_ALIAS },
{"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.

Post by Jim Wilson
{"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.

Post by Jim Wilson
{"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.

Jim Wilson

2018-11-30 02:40:12 UTC

Permalink

Post by Palmer Dabbelt

Post by Jim Wilson
{"unimp", 0, {"I", 0}, "", MATCH_CSRRW | (CSR_CYCLE << OP_SH_CSR), 0xffffffffU, match_opcode, 0 }, /* csrw cycle, x0 */

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.

Post by Palmer Dabbelt

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.

Post by Palmer Dabbelt

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

Palmer Dabbelt

2018-11-30 02:44:33 UTC

Permalink

Post by Jim Wilson

Post by Palmer Dabbelt

Post by Jim Wilson
{"unimp", 0, {"I", 0}, "", MATCH_CSRRW | (CSR_CYCLE << OP_SH_CSR), 0xffffffffU, match_opcode, 0 }, /* csrw cycle, x0 */

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.

Post by Jim Wilson
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

Post by Jim Wilson

Post by Palmer Dabbelt

I think this is also an alias, largely for the same reason.

Post by Palmer Dabbelt

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

Post by Jim Wilson

Post by Palmer Dabbelt
{"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.

Jim Wilson

2018-11-30 03:42:56 UTC

Permalink

Post by Palmer Dabbelt
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.

I'm finding your comments hard to parse, as there are too many mistakes here.

I agree that unimp is an alias for csrrw, but I think you are
unnecessarily sacrificing usability if you mark it as an alias.

Post by Palmer Dabbelt

Post by Jim Wilson
{"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.

c.ebreak is not an illegal instruction. Maybe you meant c.unimp here?

Jim