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

[Philipp Tomsich <philipp.tomsich@vrull.eu>]

On Mon, 25 Apr 2022 at 11:54, Nelson Chu <nelson.chu@sifive.com> wrote:
>
> 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 ;)

As my original patch
(https://sourceware.org/pipermail/binutils/2022-January/119236.html)
didn't have these, I'd say that this must be an artifact from "a
rebase gone wrong".  I guess this comes from the rebase mentioned in
Christoph's v2 changelog above...

> I think it would be better to follow the rules in the PR for an
> 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 :)

Which integration branch are you referring to?
AFAIK, there is no patch adding cache instructions floating around
(which is the reason why we created a cleanroom implementation for
these).

We have buy-in on the vendor rules (after all, Chen Wei from T-Head is
the vice-chair of the Software HC) from Alibaba, but you can expect
things to take a while to propagate through into their own trees and
documents.  Note that in a separate effort, I am talking to T-Head to
have their instructions grouped into multiple, separate extensions
(and hopefully also have their documentation updated to reflect this
in the same structure as we're using in the RISC-V documentation
elsewhere).

Cheers,
Philipp.


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