Re: [PATCH v1] RISC-V: Support XVentanaCondOps extension

[Palmer Dabbelt <palmer@dabbelt.com>]

On Thu, 26 May 2022 12:50:30 PDT (-0700), philipp.tomsich@vrull.eu wrote:
> On Fri, 13 May 2022 at 00:23, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Thu, 12 May 2022 12:59:58 PDT (-0700), philipp.tomsich@vrull.eu wrote:
>> > Nelson,
>> >
>> > The PR on the toolchain conventions was merged last week after weeks of
>> > discussions and everyone agreeing to these.
>> > Can we move this forward now?
>>
>> This has been a persistent problem with the RISC-V foundation.  These
>> RISC-V ports are very small parts of much larger community-oriented
>> projects with well established communication mechanisms, and trying to
>> claim that everyone was involved in discussions that happened inside the
>> RISC-V foundation just isn't accurate.  We consistently get very
>> valuable feedback from upstream contributors who chose to stick to
>> upstream-focused forums for discussion, pretending that feedback doesn't
>> exist just leads to unnecessary friction.
>>
>> There's another thread started that includes the various GNU toolchain
>> projects and proposes taking support for vendor extensions.  That is the
>> result of a handful of discussions, the RISC-V naming conventions are
>> part of that but there's a lot more than how to name extensions that's
>> proposed.  What there is the best I could come up with after talking to
>> a handful of people, happy to discuss things further.  Those discussions
>> really need to happen in the relevant forums, though, as this is a
>> really big policy change that has wide-reaching ramifications.
>
> After rereading the discussions on the other thread, I am not clear
> what the next steps are for simple (as in: does not require any custom
> relocations) vendor-defined extensions in the binutils context: are
> the existing mechanisms (i.e., Tag_RISCV_arch and making sure that the
> naming doesn't conflict) sufficient to move forward?  If not, could
> you outline the next steps and how to move forward on this?

I think that's because we didn't really finish the discussion.  I was 
intending on letting things cool off a bit, but we ended up in another 
round of silliness a few days ago so I guess it'll take a bit longer.  
This is a really important policy decision, skipping the discussion 
because we're unable to have it without devolving into a bunch of 
personal attacks against various long-term upstream contributors is the 
wrong way to do things.

As far as I can tell there's still no hardware, and given that this is 
really the poster child for breaking compatibility really don't see any 
rush -- there's certainly loads of hardware that's actually in users' 
hands that we're not supporting, that worries me way more than this.  
There's a whole host of techniques that could be used to avoid 
compatibility breaks here, that or we just agree to give up -- 
either way, that needs a proper discussion.

>
> Philipp.
>
>> > Thanks,
>> > Philipp.
>> >
>> > On Mon, 25 Apr 2022 at 11:38, Nelson Chu <nelson.chu@sifive.com> wrote:
>> >
>> >> On Sat, Apr 23, 2022 at 9:42 AM Kito Cheng <kito.cheng@sifive.com> wrote:
>> >> >
>> >> > Hi Philipp:
>> >> >
>> >> > I guess we can settle down Conventions for vendor extension[1] before
>> >> > merging this?
>> >> > We are pretty close to a consensus, no objection for the generally idea,
>> >> > just remaining minor stuffs on the prefix naming scheme,
>> >> > so I believe that could resolve soon.
>> >> >
>> >> > [1] https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
>> >>
>> >> Once the PR is merged and most of the people agree with the rules
>> >> there, I will say LGTM for the vendor extensions in general.
>> >>
>> >> Thanks
>> >> Nelson
>> >>
>> >> > On Fri, Apr 22, 2022 at 6:37 PM Philipp Tomsich
>> >> > <philipp.tomsich@vrull.eu> wrote:
>> >> > >
>> >> > > Nelson,
>> >> > >
>> >> > > Can we make some progress on this, please?
>> >> > > There are already some dependent/similar changes in the queue (e.g.
>> >> xtheadcmo)...
>> >> > >
>> >> > > Philipp.
>> >> > >
>> >> > > On Wed, 20 Apr 2022 at 14:42, Kito Cheng <kito.cheng@gmail.com> wrote:
>> >> > >>
>> >> > >> Hi Philipp:
>> >> > >>
>> >> > >> I believe we have a consensus among most GNU toolchain maintainers for
>> >> > >> accepting vendor extension to upstream / master branch,
>> >> > >> so I am +1 for this patch, but I think we still need Nelson, Palmer or
>> >> > >> Jim to give something LGTM here since I am not a binutils maintainer
>> >> > >> :)
>> >> > >>
>> >> > >> On Wed, Apr 20, 2022 at 7:38 PM Philipp Tomsich
>> >> > >> <philipp.tomsich@vrull.eu> wrote:
>> >> > >> >
>> >> > >> > Kito & Nelson,
>> >> > >> >
>> >> > >> > What is the status on this one?
>> >> > >> > Let me know if it is approved for master, so I can rebase and
>> >> commit.
>> >> > >> >
>> >> > >> > Thanks,
>> >> > >> > Philipp.
>> >> > >> >
>> >> > >> > On Sun, 9 Jan 2022 at 20:29, Philipp Tomsich <
>> >> philipp.tomsich@vrull.eu>
>> >> > >> > wrote:
>> >> > >> >
>> >> > >> > > 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.
>> >> > >> > >
>> >> > >> > > ---
>> >> > >> > >
>> >> > >> > >  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                  |  7 +++++++
>> >> > >> > >  include/opcode/riscv.h                      |  1 +
>> >> > >> > >  opcodes/riscv-opc.c                         |  4 ++++
>> >> > >> > >  7 files changed, 59 insertions(+), 2 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 8409c0254e5..39fc1e9911b 100644
>> >> > >> > > --- a/bfd/elfxx-riscv.c
>> >> > >> > > +++ b/bfd/elfxx-riscv.c
>> >> > >> > > @@ -1241,6 +1241,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,
>> >> > >> > > @@ -1248,6 +1255,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
>> >> > >> > >  };
>> >> > >> > >
>> >> > >> > > @@ -1513,8 +1521,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;
>> >> > >> > >      }
>> >> > >> > > @@ -2406,6 +2413,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 be9c1148355..1e24053cbdc 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 0b8cc6c7ddb..07c613163b7 100644
>> >> > >> > > --- a/include/opcode/riscv-opc.h
>> >> > >> > > +++ b/include/opcode/riscv-opc.h
>> >> > >> > > @@ -2029,6 +2029,11 @@
>> >> > >> > >  #define MASK_HSV_W 0xfe007fff
>> >> > >> > >  #define MATCH_HSV_D 0x6e004073
>> >> > >> > >  #define MASK_HSV_D 0xfe007fff
>> >> > >> > > +/* 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
>> >> > >> > > @@ -2628,6 +2633,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
>> >> > >> > >  /* Privileged CSRs.  */
>> >> > >> > > diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
>> >> > >> > > index 048ab0a5d68..9384c6eb84b 100644
>> >> > >> > > --- a/include/opcode/riscv.h
>> >> > >> > > +++ b/include/opcode/riscv.h
>> >> > >> > > @@ -388,6 +388,7 @@ enum riscv_insn_class
>> >> > >> > >    INSN_CLASS_V,
>> >> > >> > >    INSN_CLASS_ZVEF,
>> >> > >> > >    INSN_CLASS_SVINVAL,
>> >> > >> > > +  INSN_CLASS_XVENTANACONDOPS,
>> >> > >> > >  };
>> >> > >> > >
>> >> > >> > >  /* This structure holds information for a particular
>> >> instruction.  */
>> >> > >> > > diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
>> >> > >> > > index 2da0f7cf0a4..6c70c5b99f3 100644
>> >> > >> > > --- a/opcodes/riscv-opc.c
>> >> > >> > > +++ b/opcodes/riscv-opc.c
>> >> > >> > > @@ -1753,6 +1753,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.33.1
>> >> > >> > >
>> >> > >> > >
>> >>