Re: [PATCH RFC] mips: add TARGET_ZERO_CALL_USED_REGS hook [PR104817, PR104820]

[Richard Sandiford <richard.sandiford@arm.com>]

Xi Ruoyao <xry111@mengyan1223.wang> writes:
> Bootstrapped and regtested on mips64el-linux-gnuabi64.
>
> I'm not sure if it's "correct" to clobber other registers during the
> zeroing of scratch registers.  But I can't really come up with a better
> idea: on MIPS there is no simple way to clear one bit in FCSR (i. e.
> FCC[x]).  We can't just use "c.f.s $fccx,$f0,$f0" because it will raise
> an exception if $f0 contains a sNaN.

Yeah, it's a bit of a grey area, but I think it should be fine, provided
that the extra clobbers are never used as return registers (which is
obviously true for the FCC registers).

But on that basis…

> +static HARD_REG_SET
> +mips_zero_call_used_regs (HARD_REG_SET need_zeroed_hardregs)
> +{
> +  HARD_REG_SET zeroed_hardregs;
> +  CLEAR_HARD_REG_SET (zeroed_hardregs);
> +
> +  if (TEST_HARD_REG_BIT (need_zeroed_hardregs, HI_REGNUM))
> +    {
> +      /* Clear HI and LO altogether.  MIPS target treats HILO as a
> +	 double-word register.  */
> +      machine_mode dword_mode = TARGET_64BIT ? TImode : DImode;
> +      rtx hilo = gen_rtx_REG (dword_mode, MD_REG_FIRST);
> +      rtx zero = CONST0_RTX (dword_mode);
> +      emit_move_insn (hilo, zero);
> +
> +      SET_HARD_REG_BIT (zeroed_hardregs, HI_REGNUM);
> +      if (TEST_HARD_REG_BIT (need_zeroed_hardregs, LO_REGNUM))
> +	SET_HARD_REG_BIT (zeroed_hardregs, LO_REGNUM);
> +      else
> +	emit_clobber (gen_rtx_REG (word_mode, LO_REGNUM));

…I don't think this conditional LO_REGNUM code is worth it.
We might as well just add both registers to zeroed_hardregs.

> +    }
> +
> +  bool zero_fcc = false;
> +  for (int i = ST_REG_FIRST; i <= ST_REG_LAST; i++)
> +    if (TEST_HARD_REG_BIT (need_zeroed_hardregs, i))
> +      zero_fcc = true;
> +
> +  /* MIPS does not have a simple way to clear one bit in FCC.  We just
> +     clear FCC with ctc1 and clobber all FCC bits.  */
> +  if (zero_fcc)
> +    {
> +      emit_insn (gen_mips_zero_fcc ());
> +      for (int i = ST_REG_FIRST; i <= ST_REG_LAST; i++)
> +	if (TEST_HARD_REG_BIT (need_zeroed_hardregs, i))
> +	  SET_HARD_REG_BIT (zeroed_hardregs, i);
> +	else
> +	  emit_clobber (gen_rtx_REG (CCmode, i));
> +    }

Here too I think we should just do:

      zeroed_hardregs |= reg_class_contents[ST_REGS] & accessible_reg_set;

to include all available FCC registers.

> +
> +  need_zeroed_hardregs &= ~zeroed_hardregs;
> +  return zeroed_hardregs |
> +	 default_zero_call_used_regs (need_zeroed_hardregs);

Nit, but: should be formatted as:

  return (zeroed_hardregs
	  | default_zero_call_used_regs (need_zeroed_hardregs));

> +}
> +
>  \f
>  /* Initialize the GCC target structure.  */
>  #undef TARGET_ASM_ALIGNED_HI_OP
> @@ -22919,6 +22964,8 @@ mips_asm_file_end (void)
>  #undef TARGET_ASM_FILE_END
>  #define TARGET_ASM_FILE_END mips_asm_file_end
>  
> +#undef TARGET_ZERO_CALL_USED_REGS
> +#define TARGET_ZERO_CALL_USED_REGS mips_zero_call_used_regs
>  
>  struct gcc_target targetm = TARGET_INITIALIZER;
>  \f
> diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
> index e0f0a582732..edf58710cdd 100644
> --- a/gcc/config/mips/mips.md
> +++ b/gcc/config/mips/mips.md
> @@ -96,6 +96,7 @@ (define_c_enum "unspec" [
>    ;; Floating-point environment.
>    UNSPEC_GET_FCSR
>    UNSPEC_SET_FCSR
> +  UNSPEC_ZERO_FCC
>  
>    ;; HI/LO moves.
>    UNSPEC_MFHI
> @@ -7670,6 +7671,11 @@ (define_insn "*mips_set_fcsr"
>    "TARGET_HARD_FLOAT"
>    "ctc1\t%0,$31")
>  
> +(define_insn "mips_zero_fcc"
> +  [(unspec_volatile [(const_int 0)] UNSPEC_ZERO_FCC)]
> +  "TARGET_HARD_FLOAT"
> +  "ctc1\t$0,$25")

I've forgotten a lot of MIPS stuff, so: does this clear only the
FCC registers, or does it clear other things (such as exception bits)
as well?  Does it work even for !ISA_HAS_8CC?

I think this pattern should explicit clear all eight registers, e.g. using:

  (set (reg:CC FCC0_REGNUM) (const_int 0))
  (set (reg:CC FCC1_REGNUM) (const_int 0))
  …

which unfortunately means defining 8 new register constants in mips.md.
I guess for extra safety there should be a separate !ISA_HAS_8CC version
that only sets FCC0_REGNUM.

An alternative would be to avoid clearing the FCC registers altogether.
I suppose that's less secure, but residual information could leak through
the exception bits as well, and it isn't clear whether those should be
zeroed at the end of each function.  I guess it depends on people's
appetite for risk.

Both ways are OK with me, just mentioning it in case.

Thanks,
Richard

> +
>  ;; See tls_get_tp_mips16_<mode> for why this form is used.
>  (define_insn "mips_set_fcsr_mips16_<mode>"
>    [(unspec_volatile:SI [(match_operand:P 0 "call_insn_operand" "dS")
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-10.c b/gcc/testsuite/c-c++-common/zero-scratch-regs-10.c
> index 96e0b79b328..c23b2ceb391 100644
> --- a/gcc/testsuite/c-c++-common/zero-scratch-regs-10.c
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-10.c
> @@ -1,5 +1,5 @@
>  /* { dg-do run } */
> -/* { dg-skip-if "not implemented" { ! { i?86*-*-* x86_64*-*-* sparc*-*-* aarch64*-*-* nvptx*-*-* s390*-*-* } } } */
> +/* { dg-skip-if "not implemented" { ! { i?86*-*-* x86_64*-*-* sparc*-*-* aarch64*-*-* nvptx*-*-* s390*-*-* mips*-*-* } } } */
>  /* { dg-options "-O2" } */
>  
>  #include <assert.h>
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-11.c b/gcc/testsuite/c-c++-common/zero-scratch-regs-11.c
> index 0714f95a04f..f51f5a2161c 100644
> --- a/gcc/testsuite/c-c++-common/zero-scratch-regs-11.c
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-11.c
> @@ -1,5 +1,5 @@
>  /* { dg-do run } */
> -/* { dg-skip-if "not implemented" { ! { i?86*-*-* x86_64*-*-* sparc*-*-* aarch64*-*-* arm*-*-* nvptx*-*-* s390*-*-* } } } */
> +/* { dg-skip-if "not implemented" { ! { i?86*-*-* x86_64*-*-* sparc*-*-* aarch64*-*-* arm*-*-* nvptx*-*-* s390*-*-* mips*-*-* } } } */
>  /* { dg-options "-O2 -fzero-call-used-regs=all" } */
>  
>  #include "zero-scratch-regs-10.c"
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-8.c b/gcc/testsuite/c-c++-common/zero-scratch-regs-8.c
> index aceda7e5cb8..3e5e59b3c79 100644
> --- a/gcc/testsuite/c-c++-common/zero-scratch-regs-8.c
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-8.c
> @@ -1,5 +1,5 @@
>  /* { dg-do run } */
> -/* { dg-skip-if "not implemented" { ! { i?86*-*-* x86_64*-*-* sparc*-*-* aarch64*-*-* arm*-*-* nvptx*-*-* s390*-*-* } } } */
> +/* { dg-skip-if "not implemented" { ! { i?86*-*-* x86_64*-*-* sparc*-*-* aarch64*-*-* arm*-*-* nvptx*-*-* s390*-*-* mips*-*-* } } } */
>  /* { dg-options "-O2 -fzero-call-used-regs=all-arg" } */
>  
>  #include "zero-scratch-regs-1.c"
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-9.c b/gcc/testsuite/c-c++-common/zero-scratch-regs-9.c
> index f3152a7a732..d88d61accb2 100644
> --- a/gcc/testsuite/c-c++-common/zero-scratch-regs-9.c
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-9.c
> @@ -1,5 +1,5 @@
>  /* { dg-do run } */
> -/* { dg-skip-if "not implemented" { ! { i?86*-*-* x86_64*-*-* sparc*-*-* aarch64*-*-* arm*-*-* nvptx*-*-* s390*-*-* } } } */
> +/* { dg-skip-if "not implemented" { ! { i?86*-*-* x86_64*-*-* sparc*-*-* aarch64*-*-* arm*-*-* nvptx*-*-* s390*-*-* mips*-*-* } } } */
>  /* { dg-options "-O2 -fzero-call-used-regs=all" } */
>  
>  #include "zero-scratch-regs-1.c"