RISC-V: Add missing c.unimp instruction.

Message ID 20181129211145.19597-1-jimw@sifive.com
State New
Headers show
Series
  • RISC-V: Add missing c.unimp instruction.
Related show

Commit Message

Jim Wilson Nov. 29, 2018, 9:11 p.m.
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(-)

-- 
2.17.1

Comments

Palmer Dabbelt Nov. 30, 2018, 1:46 a.m. | #1
On Thu, 29 Nov 2018 13:11:45 PST (-0800), Jim Wilson wrote:
> 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.

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

>  {"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 Nov. 30, 2018, 2:40 a.m. | #2
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
Palmer Dabbelt Nov. 30, 2018, 2:44 a.m. | #3
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.
Jim Wilson Nov. 30, 2018, 3:42 a.m. | #4
On Thu, Nov 29, 2018 at 6:44 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> 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.

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

Patch

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 },