Re: [PATCH v2 3/3] RISC-V: PR27916, Extend .insn directive to support hardcode encoding.

Kito Cheng
2021-07-09T08:02:02Z

This patch is extending new formats of .insn, so I suggest this should
be a standalone patch.

On Fri, Jul 9, 2021 at 3:28 PM Nelson Chu <nelson.chu@sifive.com> wrote:
>
> The .insn directive can let users use their own instructions, or
> some new instruction, which haven't supported in the old binutils.
> For example, if users want to use sifive cache instruction, they
> cannot just write "cflush.d1.l1" in the assembly code, they should
> use ".insn i SYSTEM, 0, x0, x10, -0x40". But the .insn directive
> may not easy to use for some cases, and not so friendly to users.
> Therefore, I believe most of the users will use ".word 0xfc050073",
> to encode the instructions directly, rather than use .insn. But
> once we have supported the mapping symbols, the .word directives
> are marked as data, so disassembler won't dump them as instructions
> as usual. I have discussed this with Kito many times, we all think
> extend the .insn direcitve to support the hardcode encoding, is the
> easiest way to resolve the problem. Therefore, there are two more
> .insn formats are proposed as follows,
>
> (original) .insn <type>, <operand1>, <operand2>, ...
> .insn <insn-length>, <value>
> .insn <value>
>
> The <type> is string, and the <insn-length> and <value> are constants.
>
> ChangeLog:
>
> gas/
>
> pr 27916
> * config/tc-riscv.c (riscv_ip_hardcode): Similar to riscv_ip,
> but assembles an instruction according to the hardcode values
> of .insn directive.
> * testsuite/gas/riscv/insn-fail.d: New testcases.
> * testsuite/gas/riscv/insn-fail.l: Likewise.
> * testsuite/gas/riscv/insn-fail.s: Likewise.
> * testsuite/gas/riscv/insn.d: Updated.
> * testsuite/gas/riscv/insn.s: Likewise.
> ---
> gas/config/tc-riscv.c | 57 +++++++++++++++++++++++++++--
> gas/testsuite/gas/riscv/insn-fail.d | 3 ++
> gas/testsuite/gas/riscv/insn-fail.l | 7 ++++
> gas/testsuite/gas/riscv/insn-fail.s | 6 +++
> gas/testsuite/gas/riscv/insn.d | 6 +++
> gas/testsuite/gas/riscv/insn.s | 7 ++++
> 6 files changed, 83 insertions(+), 3 deletions(-)
> create mode 100644 gas/testsuite/gas/riscv/insn-fail.d
> create mode 100644 gas/testsuite/gas/riscv/insn-fail.l
> create mode 100644 gas/testsuite/gas/riscv/insn-fail.s
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 55c49471825..e9be17f56d1 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -2922,6 +2922,50 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
> return error;
> }
>
> +/* Similar to riscv_ip, but assembles an instruction according to the
> + hardcode values of .insn directive. */
> +
> +static const char *
> +riscv_ip_hardcode (char *str,
> + struct riscv_cl_insn *ip,
> + expressionS *imm_expr,
> + const char *error)
> +{
> + struct riscv_opcode *insn;
> + insn_t values[2] = {0, 0};
> + unsigned int num = 0;
> +
> + input_line_pointer = str;
> + do
> + {
> + expression (imm_expr);
> + if (imm_expr->X_op != O_constant)
> + {
> + /* The first value isn't constant, so it should be
> + .insn <type> <operands>. Call riscv_ip to parse it. */
> + if (num == 0)
> + return error;
> + return _("values must be constant");
> + }
> + values[num++] = (insn_t) imm_expr->X_add_number;
> + }
> + while (*input_line_pointer++ == ',' && num < 2);
> +
> + input_line_pointer--;
> + if (*input_line_pointer != '\0')
> + return _("unrecognized values");
> +
> + insn = XNEW (struct riscv_opcode);
> + insn->match = values[num - 1];
> + create_insn (ip, insn);
> + unsigned int bytes = riscv_insn_length (insn->match);
> + if (values[num - 1] >> (8 * bytes) != 0
> + || (num == 2 && values[0] != bytes))
> + return _("value conflicts with instruction length");
> +
> + return NULL;
> +}
> +
> void
> md_assemble (char *str)
> {
> @@ -3914,7 +3958,10 @@ s_riscv_leb128 (int sign)
> return s_leb128 (sign);
> }
>
> -/* Parse the .insn directive. */
> +/* Parse the .insn directive. There are three formats,
> + Format 1: .insn <type> <operand1>, <operand2>, ...
> + Format 2: .insn <length>, <value>
> + Format 3: .insn <value>. */
>
> static void
> s_riscv_insn (int x ATTRIBUTE_UNUSED)
> @@ -3935,11 +3982,15 @@ s_riscv_insn (int x ATTRIBUTE_UNUSED)
>
> const char *error = riscv_ip (str, &insn, &imm_expr,
> &imm_reloc, insn_type_hash);
> -
> if (error)
> {
> - as_bad ("%s `%s'", error, str);
> + char *save_in = input_line_pointer;
> + error = riscv_ip_hardcode (str, &insn, &imm_expr, error);
> + input_line_pointer = save_in;
> }
> +
> + if (error)
> + as_bad ("%s `%s'", error, str);
> else
> {
> gas_assert (insn.insn_mo->pinfo != INSN_MACRO);
> diff --git a/gas/testsuite/gas/riscv/insn-fail.d b/gas/testsuite/gas/riscv/insn-fail.d
> new file mode 100644
> index 00000000000..3548e85415a
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/insn-fail.d
> @@ -0,0 +1,3 @@
> +#as:
> +#source: insn-fail.s
> +#error_output: insn-fail.l
> diff --git a/gas/testsuite/gas/riscv/insn-fail.l b/gas/testsuite/gas/riscv/insn-fail.l
> new file mode 100644
> index 00000000000..e47d106b39b
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/insn-fail.l
> @@ -0,0 +1,7 @@
> +.*Assembler messages:
> +.*Error: unrecognized opcode `r,0x00000013'
> +.*Error: values must be constant `0x4,rs1'
> +.*Error: unrecognized values `0x4 0x5'
> +.*Error: unrecognized values `0x4,0x5,0x6'
> +.*Error: value conflicts with instruction length `0x4,0x0001'
> +.*Error: value conflicts with instruction length `0x2,0x00000013'
> diff --git a/gas/testsuite/gas/riscv/insn-fail.s b/gas/testsuite/gas/riscv/insn-fail.s
> new file mode 100644
> index 00000000000..064211d985d
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/insn-fail.s
> @@ -0,0 +1,6 @@
> + .insn r, 0x00000013
> + .insn 0x4, rs1
> + .insn 0x4 0x5
> + .insn 0x4, 0x5, 0x6
> + .insn 0x4, 0x0001
> + .insn 0x2, 0x00000013
> diff --git a/gas/testsuite/gas/riscv/insn.d b/gas/testsuite/gas/riscv/insn.d
> index 8cb3d64b1a5..4edacc63368 100644
> --- a/gas/testsuite/gas/riscv/insn.d
> +++ b/gas/testsuite/gas/riscv/insn.d
> @@ -69,3 +69,9 @@ Disassembly of section .text:
> [^:]+:[ ]+00c58533[ ]+add[ ]+a0,a1,a2
> [^:]+:[ ]+00c58533[ ]+add[ ]+a0,a1,a2
> [^:]+:[ ]+00c58533[ ]+add[ ]+a0,a1,a2
> +[^:]+:[ ]+0001[ ]+nop
> +[^:]+:[ ]+00000013[ ]+nop
> +[^:]+:[ ]+00000057[ ]+0x57
> +[^:]+:[ ]+0001[ ]+nop
> +[^:]+:[ ]+00000013[ ]+nop
> +[^:]+:[ ]+00000057[ ]+0x57
> diff --git a/gas/testsuite/gas/riscv/insn.s b/gas/testsuite/gas/riscv/insn.s
> index 937ad119ff2..84739056b1a 100644
> --- a/gas/testsuite/gas/riscv/insn.s
> +++ b/gas/testsuite/gas/riscv/insn.s
> @@ -53,3 +53,10 @@ target:
> .insn r 0x33, 0, 0, fa0, a1, fa2
> .insn r 0x33, 0, 0, a0, fa1, fa2
> .insn r 0x33, 0, 0, fa0, fa1, fa2
> +
> + .insn 0x0001
> + .insn 0x00000013
> + .insn 0x00000057
> + .insn 0x2, 0x0001
> + .insn 0x4, 0x00000013
> + .insn 0x4, 0x00000057
> --
> 2.30.2
>