Re: [PATCH] riscv: thead: Add support for the XTheadInt ISA extension

["Christoph Müllner" <christoph.muellner@vrull.eu>]

On Tue, Nov 7, 2023 at 4:04 AM Jin Ma <jinma@linux.alibaba.com> wrote:
>
> The XTheadInt ISA extension provides acceleration interruption
> instructions as defined in T-Head-specific:
>
> * th.ipush
> * th.ipop

Overall, it looks ok to me.
There are just a few small issues to clean up (see below).


>
> gcc/ChangeLog:
>
>         * config/riscv/riscv-protos.h (th_int_get_mask): New prototype.
>         (th_int_get_save_adjustment): Likewise.
>         (th_int_adjust_cfi_prologue): Likewise.
>         * config/riscv/riscv.cc (TH_INT_INTERRUPT): New macro.
>         (riscv_expand_prologue): Add the processing of XTheadInt.
>         (riscv_expand_epilogue): Likewise.
>         * config/riscv/riscv.md: New unspec.
>         * config/riscv/thead.cc (BITSET_P): New macro.
>         * config/riscv/thead.md (th_int_push): New pattern.
>         (th_int_pop): New pattern.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/xtheadint-push-pop.c: New test.
> ---
>  gcc/config/riscv/riscv-protos.h               |  3 +
>  gcc/config/riscv/riscv.cc                     | 58 +++++++++++++-
>  gcc/config/riscv/riscv.md                     |  4 +
>  gcc/config/riscv/thead.cc                     | 78 +++++++++++++++++++
>  gcc/config/riscv/thead.md                     | 67 ++++++++++++++++
>  .../gcc.target/riscv/xtheadint-push-pop.c     | 36 +++++++++
>  6 files changed, 245 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadint-push-pop.c
>
> diff --git a/gcc/config/riscv/riscv-protos.h b/gcc/config/riscv/riscv-protos.h
> index 85d4f6ed9ea..05d1fc2b3a0 100644
> --- a/gcc/config/riscv/riscv-protos.h
> +++ b/gcc/config/riscv/riscv-protos.h
> @@ -627,6 +627,9 @@ extern void th_mempair_prepare_save_restore_operands (rtx[4], bool,
>                                                       int, HOST_WIDE_INT,
>                                                       int, HOST_WIDE_INT);
>  extern void th_mempair_save_restore_regs (rtx[4], bool, machine_mode);
> +extern unsigned int th_int_get_mask(unsigned int);

Space between function name and parenthesis.

> +extern unsigned int th_int_get_save_adjustment();

Space between function name and parenthesis.
An empty parameter list should be written as "(void)".

> +extern rtx th_int_adjust_cfi_prologue (unsigned int);
>  #ifdef RTX_CODE
>  extern const char*
>  th_mempair_output_move (rtx[4], bool, machine_mode, RTX_CODE);
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 08ff05dcc3f..c623101b05e 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -101,6 +101,16 @@ along with GCC; see the file COPYING3.  If not see
>  /* True the mode switching has static frm, or false.  */
>  #define STATIC_FRM_P(c) ((c)->machine->mode_sw_info.static_frm_p)
>
> +/* True if we can use the instructions in the XTheadInt extension
> +   to handle interrupts, or false.  */
> +#define TH_INT_INTERRUPT(c)                                    \
> +  (TARGET_XTHEADINT                                            \
> +   /* The XTheadInt extension only supports rv32.  */          \
> +   && !TARGET_64BIT                                            \
> +   && (c)->machine->interrupt_handler_p                                \
> +   /* This instruction can be executed in M-mode only.*/       \

Dot, space, space, end of comment.

Maybe better:
/* The XTheadInt instructions can only be executed in M-mode.  */

> +   && (c)->machine->interrupt_mode == MACHINE_MODE)
> +
>  /* Information about a function's frame layout.  */
>  struct GTY(())  riscv_frame_info {
>    /* The size of the frame in bytes.  */
> @@ -6703,6 +6713,7 @@ riscv_expand_prologue (void)
>    unsigned fmask = frame->fmask;
>    int spimm, multi_push_additional, stack_adj;
>    rtx insn, dwarf = NULL_RTX;
> +  unsigned th_int_mask = 0;
>
>    if (flag_stack_usage_info)
>      current_function_static_stack_size = constant_lower_bound (remaining_size);
> @@ -6771,6 +6782,28 @@ riscv_expand_prologue (void)
>        REG_NOTES (insn) = dwarf;
>      }
>
> +  th_int_mask = th_int_get_mask(frame->mask);

There should be exactly one space between function name and parenthesis.

> +  if (th_int_mask && TH_INT_INTERRUPT (cfun))
> +    {
> +      frame->mask &= ~th_int_mask;
> +
> +      /* RISCV_PROLOGUE_TEMP may be used to handle some CSR for
> +        interrupts, such as fcsr. */

Dot, space, space, end of comment.

> +      if ((TARGET_HARD_FLOAT  && frame->fmask)
> +         || (TARGET_ZFINX && frame->mask))
> +       frame->mask |= (1 << RISCV_PROLOGUE_TEMP_REGNUM);
> +
> +      unsigned save_adjustment = th_int_get_save_adjustment ();
> +      frame->gp_sp_offset -= save_adjustment;
> +      remaining_size -= save_adjustment;
> +
> +      insn = emit_insn (gen_th_int_push ());
> +
> +      rtx dwarf = th_int_adjust_cfi_prologue (th_int_mask);
> +      RTX_FRAME_RELATED_P (insn) = 1;
> +      REG_NOTES (insn) = dwarf;
> +    }
> +
>    /* Save the GP, FP registers.  */
>    if ((frame->mask | frame->fmask) != 0)
>      {
> @@ -6999,6 +7032,7 @@ riscv_expand_epilogue (int style)
>      = use_multi_pop ? frame->multi_push_adj_base + frame->multi_push_adj_addi
>                     : 0;
>    rtx ra = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
> +  unsigned th_int_mask = 0;
>    rtx insn;
>
>    /* We need to add memory barrier to prevent read from deallocated stack.  */
> @@ -7161,12 +7195,32 @@ riscv_expand_epilogue (int style)
>    else if (use_restore_libcall)
>      frame->mask = 0; /* Temporarily fib that we need not restore GPRs.  */
>
> +  th_int_mask = th_int_get_mask(frame->mask);

There should be exactly one space between function name and parenthesis.

> +  if (th_int_mask && TH_INT_INTERRUPT (cfun))
> +    {
> +      frame->mask &= ~th_int_mask;
> +
> +      /* RISCV_PROLOGUE_TEMP may be used to handle some CSR for
> +        interrupts, such as fcsr. */

Dot, space, space, end of comment.

> +      if ((TARGET_HARD_FLOAT  && frame->fmask)
> +         || (TARGET_ZFINX && frame->mask))
> +       frame->mask |= (1 << RISCV_PROLOGUE_TEMP_REGNUM);
> +    }
> +
>    /* Restore the registers.  */
>    riscv_for_each_saved_v_reg (step2, riscv_restore_reg, false);
>    riscv_for_each_saved_reg (frame->total_size - step2 - libcall_size
>                               - multipop_size,
>                             riscv_restore_reg, true, style == EXCEPTION_RETURN);
>
> +  if (th_int_mask && TH_INT_INTERRUPT (cfun))
> +    {
> +      frame->mask = mask; /* Undo the above fib.  */
> +      unsigned save_adjustment = th_int_get_save_adjustment ();;

Double semi-colon.

> +      gcc_assert (step2.to_constant () >= save_adjustment);
> +      step2 -= save_adjustment;
> +    }
> +
>    if (use_restore_libcall)
>      frame->mask = mask; /* Undo the above fib.  */
>
> @@ -7229,7 +7283,9 @@ riscv_expand_epilogue (int style)
>
>        gcc_assert (mode != UNKNOWN_MODE);
>
> -      if (mode == MACHINE_MODE)
> +      if (th_int_mask && TH_INT_INTERRUPT (cfun))
> +       emit_jump_insn (gen_th_int_pop ());
> +      else if (mode == MACHINE_MODE)
>         emit_jump_insn (gen_riscv_mret ());
>        else if (mode == SUPERVISOR_MODE)
>         emit_jump_insn (gen_riscv_sret ());
> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index ae2217d0907..d4147d08838 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -126,6 +126,10 @@ (define_c_enum "unspecv" [
>    ;; XTheadFmv unspec
>    UNSPEC_XTHEADFMV
>    UNSPEC_XTHEADFMV_HW
> +
> +  ;; XTheadInt unspec
> +  UNSPECV_XTHEADINT_PUSH
> +  UNSPECV_XTHEADINT_POP
>  ])
>
>  (define_constants
> diff --git a/gcc/config/riscv/thead.cc b/gcc/config/riscv/thead.cc
> index a485fb1fba6..5b437c9a7a2 100644
> --- a/gcc/config/riscv/thead.cc
> +++ b/gcc/config/riscv/thead.cc
> @@ -36,6 +36,9 @@
>  #include "regs.h"
>  #include "riscv-protos.h"
>
> +/* True if bit BIT is set in VALUE.  */
> +#define BITSET_P(VALUE, BIT) (((VALUE) & (1ULL << (BIT))) != 0)
> +

This is copied from riscv.cc.
Let's better move it to riscv.h instead of duplicating it.

>  /* If MEM is in the form of "base+offset", extract the two parts
>     of address and set to BASE and OFFSET, otherwise return false
>     after clearing BASE and OFFSET.  */
> @@ -945,3 +948,78 @@ th_print_operand_address (FILE *file, machine_mode mode, rtx x)
>
>    gcc_unreachable ();
>  }
> +
> +/* Number array of registers X1, X5-X7, X10-X17, X28-X31, to be
> +   operated on by instruction th.ipush/th.ipop in XTheadInt.  */
> +
> +int th_int_regs[] ={
> +  RETURN_ADDR_REGNUM,
> +  T0_REGNUM, T1_REGNUM, T2_REGNUM,
> +  A0_REGNUM, A1_REGNUM, A2_REGNUM, A3_REGNUM,
> +  A4_REGNUM, A5_REGNUM, A6_REGNUM, A7_REGNUM,
> +  T3_REGNUM, T4_REGNUM, T5_REGNUM, T6_REGNUM,
> +};
> +
> +/* If MASK contains registers X1, X5-X7, X10-X17, X28-X31, then
> +   return the mask composed of these registers, otherwise return
> +   zero.  */
> +
> +unsigned int
> +th_int_get_mask(unsigned int mask)

There should be exactly one space between function name and parenthesis.

> +{
> +  unsigned int xtheadint_mask = 0;
> +
> +  if (!TARGET_XTHEADINT || TARGET_64BIT)
> +    return 0;
> +
> +  for (unsigned int i = 0; i < ARRAY_SIZE (th_int_regs); i++)
> +    {
> +      if (!BITSET_P (mask, th_int_regs[i]))
> +       return 0;
> +
> +      xtheadint_mask |= (1 << th_int_regs[i]);
> +    }
> +
> +  return xtheadint_mask; /* Usually 0xf003fce2.  */
> +}
> +
> +/* Returns the occupied frame needed to save registers X1, X5-X7,
> +   X10-X17, X28-X31.*/

Dot, space, space, end of comment.

> +
> +unsigned int
> +th_int_get_save_adjustment ()

"()" -> "(void)"

> +{
> +  gcc_assert (TARGET_XTHEADINT && !TARGET_64BIT);
> +  return ARRAY_SIZE (th_int_regs) * UNITS_PER_WORD;
> +}
> +
> +rtx
> +th_int_adjust_cfi_prologue (unsigned int mask)
> +{
> +  gcc_assert (TARGET_XTHEADINT && !TARGET_64BIT);
> +
> +  rtx dwarf = NULL_RTX;
> +  rtx adjust_sp_rtx, reg, mem, insn;
> +  int saved_size = ARRAY_SIZE (th_int_regs) * UNITS_PER_WORD;
> +  int offset = saved_size;
> +
> +  for (int regno = GP_REG_FIRST; regno <= GP_REG_LAST; regno++)
> +    if (BITSET_P (mask, regno - GP_REG_FIRST))
> +      {
> +       offset -= UNITS_PER_WORD;
> +       reg = gen_rtx_REG (SImode, regno);
> +       mem = gen_frame_mem (SImode, plus_constant (Pmode,
> +                                                   stack_pointer_rtx,
> +                                                   offset));
> +
> +       insn = gen_rtx_SET (mem, reg);
> +       dwarf = alloc_reg_note (REG_CFA_OFFSET, insn, dwarf);
> +      }
> +
> +  /* Debug info for adjust sp.  */
> +  adjust_sp_rtx = gen_rtx_SET (stack_pointer_rtx,
> +       gen_rtx_PLUS (GET_MODE(stack_pointer_rtx), stack_pointer_rtx, GEN_INT (-saved_size)));

There should be exactly one space between function name and parenthesis.

> +  dwarf = alloc_reg_note (REG_CFA_ADJUST_CFA, adjust_sp_rtx, dwarf);
> +
> +  return dwarf;
> +}
> diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
> index 2babfafb23c..4d6e16c0edc 100644
> --- a/gcc/config/riscv/thead.md
> +++ b/gcc/config/riscv/thead.md
> @@ -210,6 +210,73 @@ (define_insn "th_fmv_x_hw"
>     (set_attr "type" "fmove")
>     (set_attr "mode" "DF")])
>
> +;; XTheadInt
> +
> +(define_constants
> +  [(T0_REGNUM  5)
> +   (T1_REGNUM  6)
> +   (T2_REGNUM  7)
> +   (A0_REGNUM  10)
> +   (A1_REGNUM  11)
> +   (A2_REGNUM  12)
> +   (A3_REGNUM  13)
> +   (A4_REGNUM  14)
> +   (A5_REGNUM  15)
> +   (A6_REGNUM  16)
> +   (A7_REGNUM  17)
> +   (T3_REGNUM  28)
> +   (T4_REGNUM  29)
> +   (T5_REGNUM  30)
> +   (T6_REGNUM  31)
> +])
> +
> +(define_insn "th_int_push"
> +  [(unspec_volatile [(const_int 0)] UNSPECV_XTHEADINT_PUSH)
> +   (use (reg:SI RETURN_ADDR_REGNUM))
> +   (use (reg:SI T0_REGNUM))
> +   (use (reg:SI T1_REGNUM))
> +   (use (reg:SI T2_REGNUM))
> +   (use (reg:SI A0_REGNUM))
> +   (use (reg:SI A1_REGNUM))
> +   (use (reg:SI A2_REGNUM))
> +   (use (reg:SI A3_REGNUM))
> +   (use (reg:SI A4_REGNUM))
> +   (use (reg:SI A5_REGNUM))
> +   (use (reg:SI A6_REGNUM))
> +   (use (reg:SI A7_REGNUM))
> +   (use (reg:SI T3_REGNUM))
> +   (use (reg:SI T4_REGNUM))
> +   (use (reg:SI T5_REGNUM))
> +   (use (reg:SI T6_REGNUM))]
> +  "TARGET_XTHEADINT && !TARGET_64BIT"
> +  "th.ipush"
> +  [(set_attr "type"    "store")
> +   (set_attr "mode"    "SI")])
> +
> +(define_insn "th_int_pop"
> +  [(unspec_volatile [(const_int 0)] UNSPECV_XTHEADINT_POP)
> +   (clobber (reg:SI RETURN_ADDR_REGNUM))
> +   (clobber (reg:SI T0_REGNUM))
> +   (clobber (reg:SI T1_REGNUM))
> +   (clobber (reg:SI T2_REGNUM))
> +   (clobber (reg:SI A0_REGNUM))
> +   (clobber (reg:SI A1_REGNUM))
> +   (clobber (reg:SI A2_REGNUM))
> +   (clobber (reg:SI A3_REGNUM))
> +   (clobber (reg:SI A4_REGNUM))
> +   (clobber (reg:SI A5_REGNUM))
> +   (clobber (reg:SI A6_REGNUM))
> +   (clobber (reg:SI A7_REGNUM))
> +   (clobber (reg:SI T3_REGNUM))
> +   (clobber (reg:SI T4_REGNUM))
> +   (clobber (reg:SI T5_REGNUM))
> +   (clobber (reg:SI T6_REGNUM))
> +   (return)]
> +  "TARGET_XTHEADINT && !TARGET_64BIT"
> +  "th.ipop"
> +  [(set_attr "type"    "ret")
> +   (set_attr "mode"    "SI")])
> +
>  ;; XTheadMac
>
>  (define_insn "*th_mula<mode>"
> diff --git a/gcc/testsuite/gcc.target/riscv/xtheadint-push-pop.c b/gcc/testsuite/gcc.target/riscv/xtheadint-push-pop.c
> new file mode 100644
> index 00000000000..3383431aa30
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/xtheadint-push-pop.c
> @@ -0,0 +1,36 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv32gc_xtheadint -mabi=ilp32d" { target { rv32 } } } */
> +/* { dg-options "-march=rv64gc_xtheadint -mabi=lp64d" { target { rv64 } } } */
> +
> +extern void f(void);
> +
> +__attribute__((interrupt))
> +void func_default(void)

There should be exactly one space between function name and parenthesis.

> +{
> +  f ();
> +}
> +
> +__attribute__ ((interrupt ("machine")))
> +void func_machine(void)

There should be exactly one space between function name and parenthesis.

> +{
> +  f ();
> +}
> +
> +/* { dg-final { scan-assembler-times {\tth\.ipush\n} 2 { target { rv32 } } } } */
> +/* { dg-final { scan-assembler-times {\tth\.ipop\n} 2 { target { rv32 } } } } */
> +
> +
> +__attribute__ ((interrupt ("user")))
> +void func_usr(void)
> +{
> +  f ();
> +}
> +
> +__attribute__ ((interrupt ("supervisor")))
> +void func_supervisor(void)
> +{
> +  f ();
> +}
> +
> +/* { dg-final { scan-assembler-not {\tth\.ipush\n} { target { rv64 } } } } */
> +/* { dg-final { scan-assembler-not {\tth\.ipop\n} { target { rv64 } } } } */
> \ No newline at end of file
>
> base-commit: 2cca6ae615f3fb083d3a1e5e9dffcefd54fed990
> --
> 2.17.1
>