Re: [RFC PATCH 0/2] RISC-V: Zfinx fixes/enhancements: Part 2A

[Palmer Dabbelt <palmer@rivosinc.com>]

On Tue, 01 Feb 2022 05:51:12 PST (-0800), binutils@sourceware.org wrote:
> [About this patchset]
>
> This is the Part 2A (part 2 option A) of my Z[fdq]inx fixes and
> enhancements.  See Part 1 for general background.
>
> <https://sourceware.org/pipermail/binutils/2022-February/119570.html>
>
> There is also Part 2B (part 2 option B) I will submit later to fix the
> same bug but I prefer option A.  See below for details.
>
>
>
>
> [Bugfix in Part 2]
>
> On RV32_Zdinx and RV[32|64]_Zqinx, there is a bug that allows invalid
> Zdinx/Zqinx instructions to be emitted and/or disassembled.
>
> GNU Assembler accepts all register index (as long as encodable) but
> on Zdinx extension + RV32, all FP64 register numbers must be even
> (encoding with odd register numbers is reserved).
>
> For instance, this is valid on RV64_Zdinx but not on RV32_Zdinx.
>
>     fadd.d  a1, a3, a5
>
> On the other hand, this is valid both on RV[32|64]_Zdinx.
>
>     fadd.d  a0, a2, a4
>
> Although not ratified, Zqinx would require similar constraints (register
> pairs and quad-register groups) as the ISA Manual draft,
> section 24.5 says:
>
>> In the future, an RV64Zqinx quad-precision extension could be defined
>> analogously to RV32Zdinx.  An RV32Zqinx extension could also be
>> defined but would require quad-register groups.
>
>
>
>
> [Option A and B]
>
> To address this issue (at least in the disassembler), we need to modify
> instruction match functions to reject invalid encodings.  However,
> because whether given Zdinx/Zqinx instruction is valid depends on XLEN
> and matching extension (D/Q or Zdinx/Zqinx), current `match_func' with
> current instruction list cannot handle this situation.
>
> We have two options (as I came up with):
>
>
> ** PART 2A (OPTION A) **
> 1.  Separate D/Q and Zdinx/Zqinx completely and split Zdinx/Zqinx per
>     XLEN (because whether given Zdinx/Zqinx instruction is valid depends
>     on the XLEN).
>
> For instance,
>
>     {"fadd.d",     0, INSN_CLASS_D_OR_ZDINX,  ... , 0 },
>
> in riscv_opcodes may be splitted to:
>
>     {"fadd.d",     0, INSN_CLASS_D,      ... , 0 },
>     {"fadd.d",    32, INSN_CLASS_ZDINX,  ... , 0 },
>     {"fadd.d",    64, INSN_CLASS_ZDINX,  ... , 0 },
>
> to preserve current `match_func' interface simplicity.
>
> We also need 16 new matching functions:
>
> -   match_opcode_zdinx_rtype_g2
> -   match_opcode_zdinx_rtype_g4
>     For regular arithmetic instructions (rd, rs1 and rs2 are checked)
> -   match_rs1_eq_rs2_zdinx_rtype_g2
> -   match_rs1_eq_rs2_zdinx_rtype_g4
>     For fmv, fneg and fabs pseudoinstructions
>     (rd and rs1[==rs2] are checked).
> -   match_opcode_zdinx_r4type_g2
> -   match_opcode_zdinx_r4type_g4
>     For FMA instructions (rd and rs1..3 are checked)
> -   match_opcode_zdinx_itype_g1_2
> -   match_opcode_zdinx_itype_g1_4
> -   match_opcode_zdinx_itype_g2_1
> -   match_opcode_zdinx_itype_g2_4
> -   match_opcode_zdinx_itype_g4_1
> -   match_opcode_zdinx_itype_g4_2
>     Mainly for fcvt instructions (rd and rs1 are checked).
> -   match_opcode_zdinx_itype_g2_2
> -   match_opcode_zdinx_itype_g4_4
>     For fsqrt instructions (rd and rs1 are checked).
> -   match_opcode_zdinx_cmp_g2
> -   match_opcode_zdinx_cmp_g4
>     For compare instructions (rs1 and rs2 are checked)
>
> Downside of this is:
>
> -   It almost triples D/Q riscv_opcodes entries.  This is bloat.
> -   We need separate Zdinx/Zqinx entries per XLEN (new riscv_opcodes
>     entries will be required for RV128_Z[dq]inx).
> -   We cannot give proper diagnostics as an assembler error
>     (other than simple "illegal operands") because we purely handle this
>     issue with general-purpose matching functions.
>
>
> ** PART 2B (OPTION B) **
> 2.  Pass additional information
>
> If we prefer to give proper diagnostics in the assembler, we definitely
> need additional flags holding floating type information on each
> instruction and we need to validate all floating point register operands
> using those information.
>
> For assembler part, this is done in the Part 2B but... this part doesn't
> handle disassembler issue (unlike Part 2A).
>
> To address disassembler issue, we ADDITIONALLY need to add some
> arguments to `match_func' (RISC-V subset [riscv_parse_subset_t] and
> XLEN) and add some matching functions (like Part 2A but the number of
> new matching functions would be less) for Zdinx/Zqinx instructions.
>
> The big problem of this approach is bloat (in an other way than Part 2A)
> and redundant design.
>
> Why do we need to pass riscv_rps_dis/riscv_rps_as just for Zdinx/Zqinx
> instructions?  Why do we need to check xlen twice?  That's why I stopped
> before tackling with the disassembler issue.
>
>
>
>
> [Opinion (submitter prefers option A)]
>
> So my opinion is..., unless RISC-V maintainers prefer option B, option A
> would be better and complete.  I would like to hear thoughts of RISC-V
> people.

I'm fine with the version you have here.  At least a bit of the 
complexity could be removed by dropping Zqinx, but given that it's been 
written it doesn't seem so bad to me if we decide to keep it around.

>
>
>
>
> Tsukasa OI (2):
>   RISC-V: Prepare D/Q and Zdinx/Zqinx separation
>   RISC-V: Validate Zdinx/Zqinx register pairs
>
>  bfd/elfxx-riscv.c      |  10 +-
>  include/opcode/riscv.h |   4 +-
>  opcodes/riscv-opc.c    | 541 +++++++++++++++++++++++++++++++++--------
>  3 files changed, 441 insertions(+), 114 deletions(-)
>
>
> base-commit: e327c35ef5768789d3ba41a629f178f5eec32790
> prerequisite-patch-id: 9e408f2e6186c8956aae077daf95f38b9ad98675
> prerequisite-patch-id: 32ea143f7662a3297a7cf809cec6454e788f2916
> prerequisite-patch-id: 25d5aa65f72b1b4f1f52c92aa0f8ac30d218cc9c
> prerequisite-patch-id: 6599ccdcc15585db285c30e14528f905327fd638
> prerequisite-patch-id: a5f3689afda87a68d4faae698c438aa3211521e0