Re: [RFC PATCH v2] RISC-V: Add Zawrs ISA extension support

[Tsukasa OI <research_trasio@irq.a4lg.com>]

Functionally, LGTM.
Comments below are non-functional but related to a coding standard and
cleanliness.

On 2022/06/24 0:28, Christoph Muellner wrote:
> From: Christoph Müllner <christoph.muellner@vrull.eu>
> 
> This patch adds support for the Zawrs ISA extension
> ("wrs.nto" and "wrs.sto" instructions).
> 
> The specification can be found here:
> https://github.com/riscv/riscv-zawrs/blob/main/zawrs.adoc
> 
> Note, that the Zawrs extension is not frozen or ratified yet.
> Therefore this patch is an RFC and not intended to get merged.
> 
> Changes since v1:
> * Adjustments according to a specification change
> 
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
>  bfd/elfxx-riscv.c                  |  5 +++++
>  gas/testsuite/gas/riscv/zawrs-32.d | 11 +++++++++++
>  gas/testsuite/gas/riscv/zawrs.d    | 11 +++++++++++
>  gas/testsuite/gas/riscv/zawrs.s    |  3 +++
>  include/opcode/riscv-opc.h         |  8 ++++++++
>  include/opcode/riscv.h             |  1 +
>  opcodes/riscv-opc.c                |  4 ++++
>  7 files changed, 43 insertions(+)
>  create mode 100644 gas/testsuite/gas/riscv/zawrs-32.d
>  create mode 100644 gas/testsuite/gas/riscv/zawrs.d
>  create mode 100644 gas/testsuite/gas/riscv/zawrs.s
> 
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index f920e0ce9ff..597bdb0ee5e 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -1226,6 +1226,7 @@ static struct riscv_supported_ext riscv_supported_std_z_ext[] =
>    {"zvl16384b",		ISA_SPEC_CLASS_DRAFT,		1, 0,  0 },
>    {"zvl32768b",		ISA_SPEC_CLASS_DRAFT,		1, 0,  0 },
>    {"zvl65536b",		ISA_SPEC_CLASS_DRAFT,		1, 0,  0 },
> +  {"zawrs",		ISA_SPEC_CLASS_DRAFT,		1, 0,  0 },

As per current coding standard:

/* The standard extensions must be added in canonical order.  */

... this entry must be placed just before "zfh".  Note that this is not
a functional requirement (as list order does not affect the program) but
I think this rule is a coding standard.

>    {NULL, 0, 0, 0, 0}
>  };
>  
> @@ -2294,6 +2295,8 @@ riscv_multi_subset_supports (riscv_parse_subset_t *rps,
>      {
>      case INSN_CLASS_I:
>        return riscv_subset_supports (rps, "i");
> +    case INSN_CLASS_ZAWRS:
> +      return riscv_subset_supports (rps, "zawrs");

I prefer placing this case right after INSN_CLASS_A (not even a coding
standard but just a preference).

>      case INSN_CLASS_ZICBOM:
>        return riscv_subset_supports (rps, "zicbom");
>      case INSN_CLASS_ZICBOP:
> @@ -2410,6 +2413,8 @@ riscv_multi_subset_supports_ext (riscv_parse_subset_t *rps,
>      {
>      case INSN_CLASS_I:
>        return "i";
> +    case INSN_CLASS_ZAWRS:
> +      return "zawrs";

Same as above.

>      case INSN_CLASS_ZICSR:
>        return "zicsr";
>      case INSN_CLASS_ZIFENCEI:
> diff --git a/gas/testsuite/gas/riscv/zawrs-32.d b/gas/testsuite/gas/riscv/zawrs-32.d
> new file mode 100644
> index 00000000000..32e3a07fb3a
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/zawrs-32.d
> @@ -0,0 +1,11 @@
> +#as: -march=rv32i_zawrs
> +#source: zawrs.s
> +#objdump: -dr
> +
> +.*:[ 	]+file format .*
> +
> +Disassembly of section .text:
> +
> +0+000 <target>:
> +[ 	]+[0-9a-f]+:[ 	]+00d00073[ 	]+wrs.nto
> +[ 	]+[0-9a-f]+:[ 	]+01d00073[ 	]+wrs.sto
> diff --git a/gas/testsuite/gas/riscv/zawrs.d b/gas/testsuite/gas/riscv/zawrs.d
> new file mode 100644
> index 00000000000..9fe44f7e359
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/zawrs.d
> @@ -0,0 +1,11 @@
> +#as: -march=rv64i_zawrs
> +#source: zawrs.s
> +#objdump: -dr
> +
> +.*:[ 	]+file format .*
> +
> +Disassembly of section .text:
> +
> +0+000 <target>:
> +[ 	]+[0-9a-f]+:[ 	]+00d00073[ 	]+wrs.nto
> +[ 	]+[0-9a-f]+:[ 	]+01d00073[ 	]+wrs.sto
> diff --git a/gas/testsuite/gas/riscv/zawrs.s b/gas/testsuite/gas/riscv/zawrs.s
> new file mode 100644
> index 00000000000..138b7b5ca77
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/zawrs.s
> @@ -0,0 +1,3 @@
> +target:
> +	wrs.nto
> +	wrs.sto
> diff --git a/include/opcode/riscv-opc.h b/include/opcode/riscv-opc.h
> index 207215b79fc..bc80a1624a6 100644
> --- a/include/opcode/riscv-opc.h
> +++ b/include/opcode/riscv-opc.h
> @@ -2113,6 +2113,11 @@
>  #define MASK_CBO_INVAL 0xfff07fff
>  #define MATCH_CBO_ZERO 0x40200f
>  #define MASK_CBO_ZERO 0xfff07fff
> +/* Zawrs intructions.  */
> +#define MATCH_WRS_NTO 0x00d00073
> +#define MASK_WRS_NTO 0xffffffff
> +#define MATCH_WRS_STO 0x01d00073
> +#define MASK_WRS_STO 0xffffffff
>  /* Unprivileged Counter/Timers CSR addresses.  */
>  #define CSR_CYCLE 0xc00
>  #define CSR_TIME 0xc01
> @@ -2795,6 +2800,9 @@ DECLARE_INSN(cbo_clean, MATCH_CBO_CLEAN, MASK_CBO_CLEAN);
>  DECLARE_INSN(cbo_flush, MATCH_CBO_FLUSH, MASK_CBO_FLUSH);
>  DECLARE_INSN(cbo_inval, MATCH_CBO_INVAL, MASK_CBO_INVAL);
>  DECLARE_INSN(cbo_zero, MATCH_CBO_ZERO, MASK_CBO_ZERO);
> +/* Zawrs instructions.  */
> +DECLARE_INSN(wrs_nto, MATCH_WRS_NTO, MASK_WRS_NTO)
> +DECLARE_INSN(wrs_sto, MATCH_WRS_STO, MASK_WRS_STO)
>  #endif /* DECLARE_INSN */
>  #ifdef DECLARE_CSR
>  /* Unprivileged Counter/Timers CSRs.  */
> diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
> index 808f05f3d7a..a65b47740b8 100644
> --- a/include/opcode/riscv.h
> +++ b/include/opcode/riscv.h
> @@ -396,6 +396,7 @@ enum riscv_insn_class
>    INSN_CLASS_ZICBOP,
>    INSN_CLASS_ZICBOZ,
>    INSN_CLASS_H,
> +  INSN_CLASS_ZAWRS,
>  };
>  
>  /* This structure holds information for a particular instruction.  */
> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
> index d5cedbe176c..1f80f090bb2 100644
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -915,6 +915,10 @@ const struct riscv_opcode riscv_opcodes[] =
>  {"sfence.vma", 0, INSN_CLASS_I,    "s,t",      MATCH_SFENCE_VMA, MASK_SFENCE_VMA, match_opcode, 0 },
>  {"wfi",        0, INSN_CLASS_I,    "",         MATCH_WFI, MASK_WFI, match_opcode, 0 },
>  
> +/* Zawrs instructions.  */
> +{"wrs.nto",    0, INSN_CLASS_ZAWRS, "", MATCH_WRS_NTO, MASK_WRS_NTO, match_opcode, 0 },
> +{"wrs.sto",    0, INSN_CLASS_ZAWRS, "", MATCH_WRS_STO, MASK_WRS_STO, match_opcode, 0 },

Same as above (I prefer these lines after Zicbom and Zicboz).

> +
>  /* Zicbom and Zicboz instructions.  */
>  {"cbo.clean",  0, INSN_CLASS_ZICBOM, "0(s)", MATCH_CBO_CLEAN, MASK_CBO_CLEAN, match_opcode, 0 },
>  {"cbo.flush",  0, INSN_CLASS_ZICBOM, "0(s)", MATCH_CBO_FLUSH, MASK_CBO_FLUSH, match_opcode, 0 },