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

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

Hi Tsukasa,

On Wed, Feb 9, 2022 at 10:33 AM Tsukasa OI via Binutils
<binutils@sourceware.org> wrote:
>
> This commit adds 'Zicbop' hint instructions.
>
> bfd/ChangeLog:
>
>         * elfxx-riscv.c (riscv_multi_subset_supports): Add handling 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      | 18 ++++++++++++++++++
>  include/opcode/riscv-opc.h |  7 +++++++
>  include/opcode/riscv.h     |  1 +
>  opcodes/riscv-dis.c        |  4 ++++
>  opcodes/riscv-opc.c        |  3 +++
>  6 files changed, 35 insertions(+)
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index a7cd9320655..2bd45a0bf17 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -2336,6 +2336,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 25908597436..ddddb0989b1 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;

I prefer to reuse the q operand here.

>         case 'u': used_bits |= ENCODE_UTYPE_IMM (-1U); break;
>         case 'z': break; /* Zero immediate.  */
>         case '[': break; /* Unused operand.  */
> @@ -3163,6 +3164,23 @@ 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;
> +

Reusing the q operand should be enough.  Though we may add relocation
when the offset is a symbol, I think this should be fine.

>             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 b24e8a47c87..203bcdb9212 100644
> --- a/include/opcode/riscv-opc.h
> +++ b/include/opcode/riscv-opc.h
> @@ -2038,6 +2038,13 @@
>  #define MASK_CBO_INVAL 0xfff07fff
>  #define MATCH_CBO_ZERO 0x40200f
>  #define MASK_CBO_ZERO 0xfff07fff
> +/* Zicbop hint instructions. */
> +#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 1d74f0e521a..b769769b4ec 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 34724d4aec5..57b798d8e14 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 f092e975bd8..23716b6b7c9 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
>

Consider Christoph patch,
https://sourceware.org/pipermail/binutils/2022-January/119262.html

His patch is closer to the implementation I will do, just consider the
compatibility of amo instructions should be perfect.  However, you are
adding more testcases than us.  It would be always good to add more
testcases, so except the f operand, your patches LGTM.

Thanks
Nelson