Re: [PATCH 4/5] RISC-V: Prefetch hint instructions and operand set

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

Hi Tsukasa,

On Thu, Dec 16, 2021 at 7:11 PM Tsukasa OI via Binutils
<binutils@sourceware.org> wrote:
>
> This commit adds Zicbop hint instructions.
>
> bfd/ChangeLog:
>
>         * elfxx-riscv.c (riscv_multi_subset_supports): Add probing for
>         new instruction class.
>
> gas/ChangeLog:
>
>         * config/tc-riscv.c (riscv_ip): Add handling for new operand
>         type 'f' (32-byte aligned pseudo S-type immediate for prefetch
>         hints).
>         (validate_riscv_insn): Likewise.
>
> include/ChangeLog:
>
>         * opcode/riscv-opc.h (MATCH_PREFETCH_I, MASK_PREFETCH_I,
>         MATCH_PREFETCH_R, MASK_PREFETCH_R, MATCH_PREFETCH_W,
>         MASK_PREFETCH_W): New macros.
>         * opcode/riscv.h (enum riscv_insn_class): Add new instruction
>         class INSN_CLASS_ZICBOP.
>
> opcodes/ChangeLog:
>
>         * riscv-dis.c (print_insn_args): Add handling for new operand
>         type.
>         * riscv-opc.c (riscv_opcodes): Add prefetch hint instructions.
> ---
>  bfd/elfxx-riscv.c          |  2 ++
>  gas/config/tc-riscv.c      | 17 +++++++++++++++++
>  include/opcode/riscv-opc.h |  6 ++++++
>  include/opcode/riscv.h     |  1 +
>  opcodes/riscv-dis.c        |  4 ++++
>  opcodes/riscv-opc.c        |  3 +++
>  6 files changed, 33 insertions(+)
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index a0a043cb5a8..2efc621fa07 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -2334,6 +2334,8 @@ riscv_multi_subset_supports (riscv_parse_subset_t *rps,
>        return riscv_subset_supports (rps, "i");
>      case INSN_CLASS_ZICBOM:
>        return riscv_subset_supports (rps, "zicbom");
> +    case INSN_CLASS_ZICBOP:
> +      return riscv_subset_supports (rps, "zicbop");
>      case INSN_CLASS_ZICBOZ:
>        return riscv_subset_supports (rps, "zicboz");
>      case INSN_CLASS_ZICSR:
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index e8061217e7c..4bb5b767140 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1160,6 +1160,7 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
>         case 'a': used_bits |= ENCODE_JTYPE_IMM (-1U); break;
>         case 'p': used_bits |= ENCODE_BTYPE_IMM (-1U); break;
>         case 'q': used_bits |= ENCODE_STYPE_IMM (-1U); break;
> +       case 'f': used_bits |= ENCODE_STYPE_IMM (-1U); break;
>         case 'u': used_bits |= ENCODE_UTYPE_IMM (-1U); break;
>         case 'z': break; /* Zero immediate.  */
>         case '[': break; /* Unused operand.  */
> @@ -3163,6 +3164,22 @@ 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) & 0x1f)
> +                 || 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 (imm_expr->X_add_number);
> +             ip->insn_opcode &= ~ ENCODE_STYPE_IMM (0x1fU);

At first I felt we didn't need this line since we should already make
sure the 0-4 bits are zero of the immediate, which means the immediate
is a multiple of 32.  But if I remove this line, then I will get
multiple extra illegal operand errors, which seems redundant...  So I
spend some time, and notice that the redundant operand errors are
generated because we will call insn->match_func later.  Therefore,
masking the low 0-4 bits will let the errors look more clean.  So
LGTM.

> +             imm_expr->X_op = O_absent;
> +             asarg = expr_end;
> +             continue;
> +
>             default:
>             unknown_riscv_ip_operand:
>               as_fatal (_("internal: unknown argument type `%s'"),
> diff --git a/include/opcode/riscv-opc.h b/include/opcode/riscv-opc.h
> index c804c1afd95..ccd33ed4278 100644
> --- a/include/opcode/riscv-opc.h
> +++ b/include/opcode/riscv-opc.h
> @@ -2006,6 +2006,12 @@
>  #define MASK_CBO_INVAL 0xfff07fff
>  #define MATCH_CBO_ZERO 0x40200f
>  #define MASK_CBO_ZERO 0xfff07fff
> +#define MATCH_PREFETCH_I 0x6013
> +#define MASK_PREFETCH_I 0x1f07fff
> +#define MATCH_PREFETCH_R 0x106013
> +#define MASK_PREFETCH_R 0x1f07fff
> +#define MATCH_PREFETCH_W 0x306013
> +#define MASK_PREFETCH_W 0x1f07fff
>  /* Privileged CSR addresses.  */
>  #define CSR_USTATUS 0x0
>  #define CSR_UIE 0x4
> diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
> index 37f60a8b61c..4ff24cc21b4 100644
> --- a/include/opcode/riscv.h
> +++ b/include/opcode/riscv.h
> @@ -389,6 +389,7 @@ enum riscv_insn_class
>    INSN_CLASS_ZVEF,
>    INSN_CLASS_SVINVAL,
>    INSN_CLASS_ZICBOM,
> +  INSN_CLASS_ZICBOP,
>    INSN_CLASS_ZICBOZ,
>  };
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index d646dd56e64..07644dbca7b 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -424,6 +424,10 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info
>           print (info->stream, "%d", (int)EXTRACT_STYPE_IMM (l));
>           break;
>
> +       case 'f':
> +         print (info->stream, "%d", (int)EXTRACT_STYPE_IMM (l));
> +         break;
> +
>         case 'a':
>           info->target = EXTRACT_JTYPE_IMM (l) + pc;
>           (*info->print_address_func) (info->target, info);
> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
> index fc2a1107784..c85f1042621 100644
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -388,6 +388,9 @@ const struct riscv_opcode riscv_opcodes[] =
>  {"lw",          0, INSN_CLASS_I, "d,o(s)",    MATCH_LW, MASK_LW, match_opcode, INSN_DREF|INSN_4_BYTE },
>  {"lw",          0, INSN_CLASS_I, "d,A",       0, (int) M_LW, match_never, INSN_MACRO },
>  {"not",         0, INSN_CLASS_I, "d,s",       MATCH_XORI|MASK_IMM, MASK_XORI|MASK_IMM, match_opcode, INSN_ALIAS },
> +{"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 },
>  {"ori",         0, INSN_CLASS_I, "d,s,j",     MATCH_ORI, MASK_ORI, match_opcode, 0 },
>  {"or",          0, INSN_CLASS_C, "Cs,Cw,Ct",  MATCH_C_OR, MASK_C_OR, match_opcode, INSN_ALIAS },
>  {"or",          0, INSN_CLASS_C, "Cs,Ct,Cw",  MATCH_C_OR, MASK_C_OR, match_opcode, INSN_ALIAS },
> --
> 2.32.0
>