Re: [PATCH 1/3] RISC-V: Allocate "various" operand type

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

Nelson,

Before the real reply...

-   The reply is going to be pretty long.
-   I'm not going to reply to you today to stop myself from griping
    (not to be led by my frustrations)

Meanwhile, reviewing
<https://sourceware.org/pipermail/binutils/2022-November/124693.html>
(at least, PATCH 1-7/11 should be easy to review and partial approval of
PATCH 1-7/11 is okay to me) would be nice.

Sincerely,
Tsukasa

On 2022/11/29 11:42, Nelson Chu wrote:
> On Mon, Nov 28, 2022 at 2:39 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>>
>> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>>
>> This commit intends to move operands that require very special handling or
>> operand types that are so minor (e.g. only useful on a few instructions)
>> under "W".  I also intend this "W" to be "temporary" operand storage until
>> we can find good two character (or less) operand type.
>>
>> In this commit, prefetch offset operand "f" for 'Zicbop' extension is moved
>> to "Wif" because of its special handling (and allocating single character
>> "f" for this operand type seemed too much).
>>
>> Current expected allocation guideline is as follows:
>>
>> 1.  'W'
>> 2.  The most closely related single-letter extension in lowercase
>>     (strongly recommended but not mandatory)
>> 3.  Identify operand type
>>
>> The author currently plans to allocate following three-character operand
>> types (for operands including instructions from unratified extensions).
>>
>> 1.  "Wif" ('Zicbop': fetch offset)
> 
> Maybe just "if" without the W?  W seems redundant.  If the offset is
> immediate then using "i + <xxx>" seems more clear to understand.
> 
>> 2.  "Wfv" (unratified 'Zfa': value operand from FLI.[HSDQ] instructions)
> 
> We probably also need to modify other old operand names to comply with
> the new naming rules.  But for now the current operand names seem
> enough, maybe we can find better naming rules in the future, it's not
> urgent.
> 
>> 3.  "Wfm" / "WfM"
>>     'Zfh', 'F', 'D', 'Q': rounding modes "m" with special handling
>>                           solely for widening conversion instructions.
> 
> I still think these similar checks, including the rounding mode
> checks, should be checked in different match_opcode functions, that
> should be enough.  There is no way to make assembly wrong until you
> are using the .insn directives, and we don't need to check if the
> operands are valid or not for them, since .insn is used to allow users
> to write an unsupported instruction.  Please don't continue trying to
> add various operand names to support these, it is really hard to
> maintain.  I should have already said that, so this is the last time I
> mention it.
> 
> Nelson
> 
>> gas/ChangeLog:
>>
>>         * config/tc-riscv.c (validate_riscv_insn, riscv_ip): Move from
>>         "f" to "Wif".
>>
>> opcodes/ChangeLog:
>>
>>         * riscv-dis.c (print_insn_args): Move from "f" to "Wif".
>>         * riscv-opc.c (riscv_opcodes): Reflect new operand type.
>> ---
>>  gas/config/tc-riscv.c | 64 +++++++++++++++++++++++++++++++------------
>>  opcodes/riscv-dis.c   | 26 ++++++++++++++----
>>  opcodes/riscv-opc.c   |  6 ++--
>>  3 files changed, 71 insertions(+), 25 deletions(-)
>>
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index 0682eb355241..bb0e18ac8d52 100644
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -1359,7 +1359,6 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>>         case 'j': used_bits |= ENCODE_ITYPE_IMM (-1U); break;
>>         case 'a': used_bits |= ENCODE_JTYPE_IMM (-1U); break;
>>         case 'p': used_bits |= ENCODE_BTYPE_IMM (-1U); break;
>> -       case 'f': /* Fall through.  */
>>         case 'q': used_bits |= ENCODE_STYPE_IMM (-1U); break;
>>         case 'u': used_bits |= ENCODE_UTYPE_IMM (-1U); break;
>>         case 'z': break; /* Zero immediate.  */
>> @@ -1386,6 +1385,21 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>>                 goto unknown_validate_operand;
>>             }
>>           break;
>> +       case 'W': /* Various operands.  */
>> +         switch (*++oparg)
>> +           {
>> +           case 'i':
>> +             switch (*++oparg)
>> +               {
>> +               case 'f': used_bits |= ENCODE_STYPE_IMM (-1U); break;
>> +               default:
>> +                 goto unknown_validate_operand;
>> +               }
>> +             break;
>> +           default:
>> +             goto unknown_validate_operand;
>> +           }
>> +         break;
>>         case 'X': /* Integer immediate.  */
>>           {
>>             size_t n;
>> @@ -3401,22 +3415,37 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
>>               imm_expr->X_op = O_absent;
>>               continue;
>>
>> -           case 'f': /* Prefetch offset, pseudo S-type but lower 5-bits zero.  */
>> -             if (riscv_handle_implicit_zero_offset (imm_expr, asarg))
>> -               continue;
>> -             my_getExpression (imm_expr, asarg);
>> -             check_absolute_expr (ip, imm_expr, false);
>> -             if (((unsigned) (imm_expr->X_add_number) & 0x1fU)
>> -                 || imm_expr->X_add_number >= (signed) RISCV_IMM_REACH / 2
>> -                 || imm_expr->X_add_number < -(signed) RISCV_IMM_REACH / 2)
>> -               as_bad (_("improper prefetch offset (%ld)"),
>> -                       (long) imm_expr->X_add_number);
>> -             ip->insn_opcode |=
>> -               ENCODE_STYPE_IMM ((unsigned) (imm_expr->X_add_number) &
>> -                                 ~ 0x1fU);
>> -             imm_expr->X_op = O_absent;
>> -             asarg = expr_end;
>> -             continue;
>> +           case 'W': /* Various operands.  */
>> +             switch (*++oparg)
>> +               {
>> +               case 'i':
>> +                 switch (*++oparg)
>> +                   {
>> +                   case 'f':
>> +                     /* Prefetch offset for 'Zicbop' extension.
>> +                        pseudo S-type but lower 5-bits zero.  */
>> +                     if (riscv_handle_implicit_zero_offset (imm_expr, asarg))
>> +                       continue;
>> +                     my_getExpression (imm_expr, asarg);
>> +                     check_absolute_expr (ip, imm_expr, false);
>> +                     if (((unsigned) (imm_expr->X_add_number) & 0x1fU)
>> +                         || imm_expr->X_add_number >= RISCV_IMM_REACH / 2
>> +                         || imm_expr->X_add_number < -RISCV_IMM_REACH / 2)
>> +                       as_bad (_ ("improper prefetch offset (%ld)"),
>> +                               (long) imm_expr->X_add_number);
>> +                     ip->insn_opcode |= ENCODE_STYPE_IMM (
>> +                         (unsigned) (imm_expr->X_add_number) & ~0x1fU);
>> +                     imm_expr->X_op = O_absent;
>> +                     asarg = expr_end;
>> +                     continue;
>> +                   default:
>> +                     goto unknown_riscv_ip_operand;
>> +                   }
>> +                 break;
>> +               default:
>> +                 goto unknown_riscv_ip_operand;
>> +               }
>> +             break;
>>
>>             case 'X': /* Integer immediate.  */
>>               {
>> @@ -3469,6 +3498,7 @@ riscv_ip (char *str, struct riscv_cl_insn *ip, expressionS *imm_expr,
>>                   }
>>               }
>>               break;
>> +
>>             default:
>>             unknown_riscv_ip_operand:
>>               as_fatal (_("internal: unknown argument type `%s'"),
>> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
>> index 0e1f3b4610aa..1e6716e8e58c 100644
>> --- a/opcodes/riscv-dis.c
>> +++ b/opcodes/riscv-dis.c
>> @@ -473,11 +473,6 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
>>                  (int)EXTRACT_STYPE_IMM (l));
>>           break;
>>
>> -       case 'f':
>> -         print (info->stream, dis_style_address_offset, "%d",
>> -                (int)EXTRACT_STYPE_IMM (l));
>> -         break;
>> -
>>         case 'a':
>>           info->target = EXTRACT_JTYPE_IMM (l) + pc;
>>           (*info->print_address_func) (info->target, info);
>> @@ -582,6 +577,27 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
>>           print (info->stream, dis_style_immediate, "%d", rs1);
>>           break;
>>
>> +       case 'W': /* Various operands.  */
>> +         {
>> +           switch (*++oparg)
>> +             {
>> +             case 'i':
>> +               switch (*++oparg)
>> +                 {
>> +                 case 'f':
>> +                   print (info->stream, dis_style_address_offset, "%d",
>> +                          (int) EXTRACT_STYPE_IMM (l));
>> +                   break;
>> +                 default:
>> +                   goto undefined_modifier;
>> +                 }
>> +                 break;
>> +             default:
>> +               goto undefined_modifier;
>> +             }
>> +         }
>> +         break;
>> +
>>         case 'X': /* Integer immediate.  */
>>           {
>>             size_t n;
>> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
>> index 0e691544f9bc..653eb60f2a58 100644
>> --- a/opcodes/riscv-opc.c
>> +++ b/opcodes/riscv-opc.c
>> @@ -313,9 +313,9 @@ const struct riscv_opcode riscv_opcodes[] =
>>  /* name, xlen, isa, operands, match, mask, match_func, pinfo.  */
>>
>>  /* Standard hints.  */
>> -{"prefetch.i",  0, INSN_CLASS_ZICBOP, "f(s)", MATCH_PREFETCH_I, MASK_PREFETCH_I, match_opcode, 0 },
>> -{"prefetch.r",  0, INSN_CLASS_ZICBOP, "f(s)", MATCH_PREFETCH_R, MASK_PREFETCH_R, match_opcode, 0 },
>> -{"prefetch.w",  0, INSN_CLASS_ZICBOP, "f(s)", MATCH_PREFETCH_W, MASK_PREFETCH_W, match_opcode, 0 },
>> +{"prefetch.i",  0, INSN_CLASS_ZICBOP, "Wif(s)", MATCH_PREFETCH_I, MASK_PREFETCH_I, match_opcode, 0 },
>> +{"prefetch.r",  0, INSN_CLASS_ZICBOP, "Wif(s)", MATCH_PREFETCH_R, MASK_PREFETCH_R, match_opcode, 0 },
>> +{"prefetch.w",  0, INSN_CLASS_ZICBOP, "Wif(s)", MATCH_PREFETCH_W, MASK_PREFETCH_W, match_opcode, 0 },
>>  {"pause",       0, INSN_CLASS_ZIHINTPAUSE, "", MATCH_PAUSE, MASK_PAUSE, match_opcode, 0 },
>>
>>  /* Basic RVI instructions and aliases.  */
>> --
>> 2.38.1
>>
>