Re: [PATCH 1/2] RISC-V: Support XVentanaCondOps extension

[Nelson Chu <nelson.chu@sifive.com>]

On Wed, Apr 20, 2022 at 10:56 PM Christoph Muellner
<cmuellner@gcc.gnu.org> wrote:
>
> From: Philipp Tomsich <philipp.tomsich@vrull.eu>
>
> Ventana Micro has published the specification for their
> XVentanaCondOps ("conditional ops") extension at
>   https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> which contains two new instructions
>   - vt.maskc
>   - vt.maskcn
> that can be used in constructing branchless sequences for
> various conditional-arithmetic, conditional-logical, and
> conditional-select operations.
>
> To support such vendor-defined instructions in the mainline binutils,
> this change also adds a riscv_supported_vendor_x_ext secondary
> dispatch table (but also keeps the behaviour of allowing any unknow
> X-extension to be specified in addition to the known ones from this
> table).
>
> As discussed, this change already includes the planned/agreed future
> requirements for X-extensions (which are likely to be captured in the
> riscv-toolchain-conventions repository):
>   - a public specification document is available (see above) and is
>     referenced from the gas-documentation
>   - the naming follows chapter 27 of the RISC-V ISA specification
>   - instructions are prefixed by a vendor-prefix (vt for Ventana)
>     to ensure that they neither conflict with future standard
>     extensions nor clash with other vendors
>
> bfd/ChangeLog:
>
>         * elfxx-riscv.c (riscv_get_default_ext_version): Add riscv_supported_vendor_x_ext.
>         (riscv_multi_subset_supports): Recognize INSN_CLASS_XVENTANACONDOPS.
>
> gas/ChangeLog:
>
>         * doc/c-riscv.texi: Add section to list custom extensions and
>           their documentation URLs.
>         * testsuite/gas/riscv/x-ventana-condops.d: New test.
>         * testsuite/gas/riscv/x-ventana-condops.s: New test.
>
> include/ChangeLog:
>
>         * opcode/riscv-opc.h Add vt.maskc and vt.maskcn.
>         * opcode/riscv.h (enum riscv_insn_class): Add INSN_CLASS_XVENTANACONDOPS.
>
> opcodes/ChangeLog:
>
>         * riscv-opc.c: Add vt.maskc and vt.maskcn.
>
> v2:
> - Rebase (no changes requested for v1; see
>   https://sourceware.org/pipermail/binutils/2022-January/119236.html)
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> ---
>  bfd/elfxx-riscv.c                           | 13 +++++++++++--
>  gas/doc/c-riscv.texi                        | 20 ++++++++++++++++++++
>  gas/testsuite/gas/riscv/x-ventana-condops.d | 12 ++++++++++++
>  gas/testsuite/gas/riscv/x-ventana-condops.s |  4 ++++
>  include/opcode/riscv-opc.h                  | 17 ++++++++++++++++-
>  include/opcode/riscv.h                      |  1 +
>  opcodes/riscv-opc.c                         |  4 ++++
>  7 files changed, 68 insertions(+), 3 deletions(-)
>  create mode 100644 gas/testsuite/gas/riscv/x-ventana-condops.d
>  create mode 100644 gas/testsuite/gas/riscv/x-ventana-condops.s
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index cb2cc146c04..723b30ddbfc 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -1237,6 +1237,13 @@ static struct riscv_supported_ext riscv_supported_std_zxm_ext[] =
>    {NULL, 0, 0, 0, 0}
>  };
>
> +static struct riscv_supported_ext riscv_supported_vendor_x_ext[] =
> +{
> +  /* XVentanaCondOps: https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf */
> +  {"xventanacondops",  ISA_SPEC_CLASS_DRAFT,   1, 0, 0 },
> +  {NULL, 0, 0, 0, 0}
> +};
> +
>  const struct riscv_supported_ext *riscv_all_supported_ext[] =
>  {
>    riscv_supported_std_ext,
> @@ -1244,6 +1251,7 @@ const struct riscv_supported_ext *riscv_all_supported_ext[] =
>    riscv_supported_std_s_ext,
>    riscv_supported_std_h_ext,
>    riscv_supported_std_zxm_ext,
> +  riscv_supported_vendor_x_ext,
>    NULL
>  };
>
> @@ -1504,8 +1512,7 @@ riscv_get_default_ext_version (enum riscv_spec_class *default_isa_spec,
>      case RV_ISA_CLASS_Z: table = riscv_supported_std_z_ext; break;
>      case RV_ISA_CLASS_S: table = riscv_supported_std_s_ext; break;
>      case RV_ISA_CLASS_H: table = riscv_supported_std_h_ext; break;
> -    case RV_ISA_CLASS_X:
> -      break;
> +    case RV_ISA_CLASS_X: table = riscv_supported_vendor_x_ext; break;
>      default:
>        table = riscv_supported_std_ext;
>      }
> @@ -2402,6 +2409,8 @@ riscv_multi_subset_supports (riscv_parse_subset_t *rps,
>               || riscv_subset_supports (rps, "zve32f"));
>      case INSN_CLASS_SVINVAL:
>        return riscv_subset_supports (rps, "svinval");
> +    case INSN_CLASS_XVENTANACONDOPS:
> +      return riscv_subset_supports (rps, "xventanacondops");
>      default:
>        rps->error_handler
>          (_("internal: unreachable INSN_CLASS_*"));
> diff --git a/gas/doc/c-riscv.texi b/gas/doc/c-riscv.texi
> index 21d867e9cf0..c75a5ad5a08 100644
> --- a/gas/doc/c-riscv.texi
> +++ b/gas/doc/c-riscv.texi
> @@ -20,6 +20,7 @@
>  * RISC-V-Modifiers::      RISC-V Assembler Modifiers
>  * RISC-V-Formats::        RISC-V Instruction Formats
>  * RISC-V-ATTRIBUTE::      RISC-V Object Attribute
> +* RISC-V-CustomExts::     RISC-V Custom (Vendor-Defined) Extensions
>  @end menu
>
>  @node RISC-V-Options
> @@ -692,3 +693,22 @@ the privileged specification.  It will report errors if object files of
>  different privileged specification versions are merged.
>
>  @end table
> +
> +@node RISC-V-CustomExts
> +@section RISC-V Custom (Vendor-Defined) Extensions
> +@cindex custom (vendor-defined) extensions, RISC-V
> +@cindex RISC-V custom (vendor-defined) extensions
> +
> +The following table lists the custom (vendor-defined) RISC-V
> +extensions supported and provides the location of their
> +publicly-released documentation:
> +
> +@table @r
> +@item XVentanaCondOps
> +XVentanaCondOps extension provides instructions for branchless
> +sequences that perform conditional arithmetic, conditional
> +bitwise-logic, and conditional select operations.
> +
> +It is documented at @url{https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf}.
> +
> +@end table
> diff --git a/gas/testsuite/gas/riscv/x-ventana-condops.d b/gas/testsuite/gas/riscv/x-ventana-condops.d
> new file mode 100644
> index 00000000000..cab0cc8dc12
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/x-ventana-condops.d
> @@ -0,0 +1,12 @@
> +#as: -march=rv64i_xventanacondops1p0
> +#source: x-ventana-condops.s
> +#objdump: -d
> +
> +.*:[   ]+file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+000 <target>:
> +[      ]+0:[   ]+00c5e57b[     ]+vt.maskc[     ]+a0,a1,a2
> +[      ]+4:[   ]+00e6f57b[     ]+vt.maskcn[    ]+a0,a3,a4
> diff --git a/gas/testsuite/gas/riscv/x-ventana-condops.s b/gas/testsuite/gas/riscv/x-ventana-condops.s
> new file mode 100644
> index 00000000000..562cf7384f7
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/x-ventana-condops.s
> @@ -0,0 +1,4 @@
> +target:
> +       vt.maskc        a0, a1, a2
> +       vt.maskcn       a0, a3, a4
> +
> diff --git a/include/opcode/riscv-opc.h b/include/opcode/riscv-opc.h
> index 3eea33a5dae..419ed538da9 100644
> --- a/include/opcode/riscv-opc.h
> +++ b/include/opcode/riscv-opc.h
> @@ -2045,7 +2045,20 @@
>  #define MASK_CBO_INVAL 0xfff07fff
>  #define MATCH_CBO_ZERO 0x40200f
>  #define MASK_CBO_ZERO 0xfff07fff
> -/* Unprivileged Counter/Timers CSR addresses.  */
> +/* Vendor-specific (Ventana Microsystems) XVentanaCondOps instructions */
> +#define MATCH_VT_MASKC 0x607b
> +#define MASK_VT_MASKC 0xfe00707f
> +#define MATCH_VT_MASKCN 0x707b
> +#define MASK_VT_MASKCN 0xfe00707f
> +/* Privileged CSR addresses.  */
> +#define CSR_USTATUS 0x0
> +#define CSR_UIE 0x4
> +#define CSR_UTVEC 0x5
> +#define CSR_USCRATCH 0x40
> +#define CSR_UEPC 0x41
> +#define CSR_UCAUSE 0x42
> +#define CSR_UTVAL 0x43
> +#define CSR_UIP 0x44

These N-ext CSRs should be removed since priv 1.12 spec, so it seems
like they are added by accident here ;)

I think it would be better to follow the rules in the PR for any
RISC-V vendor extension
(https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17).
So once the pr is approved and merged, I will say LGTM for this patch
and am happy to support Ventana's vendor extension in the master
branch.

As for the t-head cache instruction, personally for me, it would be
better to move the whole support from the integration branch to
master, and keep the Alibaba's guys as the main author.  If they agree
with the vendor rules in the PR above, and could help to rebase their
patch, it would be great :)

Thanks
Nelson

>  #define CSR_CYCLE 0xc00
>  #define CSR_TIME 0xc01
>  #define CSR_INSTRET 0xc02
> @@ -2720,6 +2733,8 @@ DECLARE_INSN(hsv_b, MATCH_HSV_B, MASK_HSV_B)
>  DECLARE_INSN(hsv_h, MATCH_HSV_H, MASK_HSV_H)
>  DECLARE_INSN(hsv_w, MATCH_HSV_W, MASK_HSV_W)
>  DECLARE_INSN(hsv_d, MATCH_HSV_D, MASK_HSV_D)
> +DECLARE_INSN(vt_maskc, MATCH_VT_MASKC, MASK_VT_MASKC)
> +DECLARE_INSN(vt_maskcn, MATCH_VT_MASKCN, MASK_VT_MASKCN)
>  #endif /* DECLARE_INSN */
>  #ifdef DECLARE_CSR
>  /* Unprivileged Counter/Timers CSRs.  */
> diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
> index b769769b4ec..3cbb68b5655 100644
> --- a/include/opcode/riscv.h
> +++ b/include/opcode/riscv.h
> @@ -391,6 +391,7 @@ enum riscv_insn_class
>    INSN_CLASS_ZICBOM,
>    INSN_CLASS_ZICBOP,
>    INSN_CLASS_ZICBOZ,
> +  INSN_CLASS_XVENTANACONDOPS,
>  };
>
>  /* This structure holds information for a particular instruction.  */
> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
> index 00108ff24ae..052209f6fe2 100644
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -1762,6 +1762,10 @@ const struct riscv_opcode riscv_opcodes[] =
>  {"hsv.w",       0, INSN_CLASS_I, "t,0(s)", MATCH_HSV_W, MASK_HSV_W, match_opcode, INSN_DREF|INSN_4_BYTE },
>  {"hsv.d",      64, INSN_CLASS_I, "t,0(s)", MATCH_HSV_D, MASK_HSV_D, match_opcode, INSN_DREF|INSN_8_BYTE },
>
> +/* Vendor-specific (Ventana Microsystems) XVentanaCondOps instructions */
> +{"vt.maskc",    0, INSN_CLASS_XVENTANACONDOPS, "d,s,t", MATCH_VT_MASKC, MASK_VT_MASKC, match_opcode, 0 },
> +{"vt.maskcn",   0, INSN_CLASS_XVENTANACONDOPS, "d,s,t", MATCH_VT_MASKCN, MASK_VT_MASKCN, match_opcode, 0 },
> +
>  /* Terminate the list.  */
>  {0, 0, INSN_CLASS_NONE, 0, 0, 0, 0, 0}
>  };
> --
> 2.35.1
>