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

  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.

--

This fixes the ICEs using -fzero-call-used-regs=all for MIPS target.

OpenSSH-8.9p1 has started to enable this by default, giving us a reason
to fix -fzero-call-used-regs for more targets.

gcc/

	PR target/104817
	PR target/104820
	* config/mips/mips.cc (mips_zero_call_used_regs): New function.
	  (TARGET_ZERO_CALL_USED_REGS): Define.
	* config/mips/mips.md (mips_zero_fcc): New insn.

gcc/testsuite

	* c-c++-common/zero-scratch-regs-8.c: Enable for MIPS.
	* c-c++-common/zero-scratch-regs-9.c: Likewise.
	* c-c++-common/zero-scratch-regs-10.c: Likewise.
	* c-c++-common/zero-scratch-regs-11.c: Likewise.
---
 gcc/config/mips/mips.cc                       | 47 +++++++++++++++++++
 gcc/config/mips/mips.md                       |  6 +++
 .../c-c++-common/zero-scratch-regs-10.c       |  2 +-
 .../c-c++-common/zero-scratch-regs-11.c       |  2 +-
 .../c-c++-common/zero-scratch-regs-8.c        |  2 +-
 .../c-c++-common/zero-scratch-regs-9.c        |  2 +-
 6 files changed, 57 insertions(+), 4 deletions(-)
  

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));

> +}
> +
>  
>  /* 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;
>  
> 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"
  

On Wed, 2022-03-09 at 18:25 +0000, Richard Sandiford wrote:
> 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…

/* snip */

> > +      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.

(See below)

> > +    }
> > +
> > +  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.

I'm afraid that doing so will cause an ICE (triggering an assertion
somewhere).  Could someone confirm that returning "more" registers than
required is allowed?  GCC Internal does not say it explicitly, and x86
port is carefully avoiding from clearing registers not requested to be
cleared.

> > +  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));
> 
> > +}

Will do.

> >  /* 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;
> >  
> > 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?

Yes, with fs = 25 CTC1 only clear FCCs.

> Does it work even for !ISA_HAS_8CC?

For !ISA_HAS_8CC targets, ST_REG_FIRST is not added into
need_zeroed_hardregs at all. I think it's another bug I didn't
catched...

> 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.

Will do.

> 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.
  

Changes from v1:

 * Added all zeroed registers into the return value of
   TARGET_ZERO_CALL_USED_REGS.  **The question: is this allowed?**
 * Define mips_zero_fcc insn only for ISA_HAS_8CC and mips_isa >
   MIPS_ISA_MIPS4, because MIPS I-IV and MIPSr6 don't support
   "ISA_HAS_8CC && mips_isa > MIPS_ISA_MIPS4".
 * Change mips_zero_fcc to explicit clear all eight registers.
 * Report an error for MIPS I-IV.

-- >8 --

This fixes the ICEs using -fzero-call-used-regs=all for MIPS target.

OpenSSH-8.9p1 has started to enable this by default, giving us a reason
to fix -fzero-call-used-regs for more targets.

gcc/

 PR target/xxxxxx (WIP)
 PR target/xxxxxx (Don't push)
 * config/mips/mips.cc (mips_zero_call_used_regs): New function.
 (TARGET_ZERO_CALL_USED_REGS): Define.
 * config/mips/mips.md (FCC{0..9}_REGNUM): New constants.
 (mips_zero_fcc): New insn.

gcc/testsuite

 * c-c++-common/zero-scratch-regs-8.c: Enable for MIPS.
 * c-c++-common/zero-scratch-regs-9.c: Likewise.
 * c-c++-common/zero-scratch-regs-10.c: Likewise.
 * c-c++-common/zero-scratch-regs-11.c: Likewise.
---
 gcc/config/mips/mips.cc | 55 +++++++++++++++++++
 gcc/config/mips/mips.md | 20 +++++++
 .../c-c++-common/zero-scratch-regs-10.c | 2 +-
 .../c-c++-common/zero-scratch-regs-11.c | 2 +-
 .../c-c++-common/zero-scratch-regs-8.c | 2 +-
 .../c-c++-common/zero-scratch-regs-9.c | 2 +-
 6 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index 4f9683e8bf4..59eef515826 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -22611,6 +22611,59 @@ mips_asm_file_end (void)
 if (NEED_INDICATE_EXEC_STACK)
 file_end_indicate_exec_stack ();
 }
+
+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);
+ SET_HARD_REG_BIT (zeroed_hardregs, LO_REGNUM);
+ }
+
+ /* MIPS does not have a simple way to clear one bit in FCC. We just
+ clear FCC with ctc1 and clobber all FCC bits. */
+ HARD_REG_SET fcc = reg_class_contents[ST_REGS] & accessible_reg_set;
+ if (hard_reg_set_intersect_p (need_zeroed_hardregs, fcc))
+ {
+ static bool issued_error = false;
+ if (mips_isa <= MIPS_ISA_MIPS4)
+ {
+ /* We don't have an easy way to clear FCC on MIPS I, II, III,
+ and IV. */
+ if (!issued_error)
+ sorry ("%qs not supported on this target",
+ "-fzero-call-used-regs");
+ issued_error = true;
+
+ /* Prevent an ICE. */
+ need_zeroed_hardregs &= ~fcc;
+ }
+ else
+ {
+ /* If the target is MIPSr6, we should not reach here. All other
+ MIPS targets are ISA_HAS_8CC. */
+ gcc_assert (ISA_HAS_8CC);
+ emit_insn (gen_mips_zero_fcc ());
+ zeroed_hardregs |= fcc;
+ }
+ }
+
+ need_zeroed_hardregs &= ~zeroed_hardregs;
+ return (zeroed_hardregs |
+ default_zero_call_used_regs (need_zeroed_hardregs));
+}
+
 
 /* Initialize the GCC target structure. */
 #undef TARGET_ASM_ALIGNED_HI_OP
@@ -22919,6 +22972,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;
 
diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index e0f0a582732..36d6a43d67f 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -167,6 +167,14 @@ (define_constants
 (SET_FCSR_REGNUM 4)
 (PIC_FUNCTION_ADDR_REGNUM 25)
 (RETURN_ADDR_REGNUM 31)
+ (FCC0_REGNUM 67)
+ (FCC1_REGNUM 68)
+ (FCC2_REGNUM 69)
+ (FCC3_REGNUM 70)
+ (FCC4_REGNUM 71)
+ (FCC5_REGNUM 72)
+ (FCC6_REGNUM 73)
+ (FCC7_REGNUM 74)
 (CPRESTORE_SLOT_REGNUM 76)
 (GOT_VERSION_REGNUM 79)
@@ -7670,6 +7678,18 @@ (define_insn "*mips_set_fcsr"
 "TARGET_HARD_FLOAT"
 "ctc1\t%0,$31")
+(define_insn "mips_zero_fcc"
+ [(set (reg:CC FCC0_REGNUM) (const_int 0))
+ (set (reg:CC FCC1_REGNUM) (const_int 0))
+ (set (reg:CC FCC2_REGNUM) (const_int 0))
+ (set (reg:CC FCC3_REGNUM) (const_int 0))
+ (set (reg:CC FCC4_REGNUM) (const_int 0))
+ (set (reg:CC FCC5_REGNUM) (const_int 0))
+ (set (reg:CC FCC6_REGNUM) (const_int 0))
+ (set (reg:CC FCC7_REGNUM) (const_int 0))]
+ "TARGET_HARD_FLOAT && ISA_HAS_8CC && mips_isa > MIPS_ISA_MIPS4"
+ "ctc1\t$0,$25")
+
 ;; 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"
  

> On Mar 9, 2022, at 12:25 PM, Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> 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.

If the LO_REGNUM is NOT in “need_zeroed_hardregs”, adding it to “zeroed_hardregs” seems not right to me.
What’s you mean by “not worth it”?

> 
>> +    }
>> +
>> +  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.

What’s the relationship between “ST_REGs” and FCC? (sorry for the stupid question since I am not familiar with the MIPS register set).

From the above code, looks like that when any  “ST_REGs” is in “need_zeroed_hardregs”,FCC need to be cleared? 

thanks.

Qing


> 
>> +
>> +  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));
> 
>> +}
>> +
>> 
>> /* 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;
>> 
>> 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"
  

On Thu, 2022-03-10 at 20:31 +0000, Qing Zhao wrote:

> > > +      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.
> 
> If the LO_REGNUM is NOT in “need_zeroed_hardregs”, adding it to “zeroed_hardregs” seems not right to me.
> What’s you mean by “not worth it”?

It's because the MIPS port almost always treat HI as "a subreg of dword
HI-LO register".  A direct "mthi $0" is possible but MIPS backend does
not recognize "emit_move_insn (HI, CONST_0)".  In theory it's possible
to emit the mthi instruction explicitly here though, but we'll need to
clear something NOT in need_zeroed_hardregs for MIPS anyway (see below).

> > Here too I think we should just do:
> > 
> >      zeroed_hardregs |= reg_class_contents[ST_REGS] & accessible_reg_set;
> > 
> > to include all available FCC registers.
> 
> What’s the relationship between “ST_REGs” and FCC? (sorry for the stupid question since I am not familiar with the MIPS register set).

MIPS instruction manual names the 8 one-bit floating condition codes
FCC0, ..., FCC7, but GCC MIPS backend code names the condition codes
ST_REG0, ..., ST_REG7.  Maybe it's better to always use the name
"ST_REG" instead of "FCC" then.

> From the above code, looks like that when any  “ST_REGs” is in “need_zeroed_hardregs”,FCC need to be cleared? 

Because there is no elegant way to clear one specific FCC bit in MIPS. 
A "ctc1 $0, $25" instruction will zero them altogether.  If we really
need to clear only one of them (let's say ST_REG3), we'll have to emit
something like

mtc1  $0, $0           # zero FPR0 to ensure it won't contain sNaN
c.f.s $3, $0, $0

Then we'll still need to clobber FPR0 with zero.  So anyway we'll have
to clear some registers not specified in need_zeroed_hardregs.

And the question is: is it really allowed to return something other than
a subset of need_zeroed_hardregs for a TARGET_ZERO_CALL_USED_REGS hook?
If yes then we'll happily to do so (like how the v2 of the patch does),
otherwise we'd need to clobber those registers NOT in
need_zeroed_hardregs explicitly.
  

> On Mar 10, 2022, at 8:54 PM, Xi Ruoyao <xry111@mengyan1223.wang> wrote:
> 
> On Thu, 2022-03-10 at 20:31 +0000, Qing Zhao wrote:
> 
>>>> +      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.
>> 
>> If the LO_REGNUM is NOT in “need_zeroed_hardregs”, adding it to “zeroed_hardregs” seems not right to me.
>> What’s you mean by “not worth it”?
> 
> It's because the MIPS port almost always treat HI as "a subreg of dword
> HI-LO register".  A direct "mthi $0" is possible but MIPS backend does
> not recognize "emit_move_insn (HI, CONST_0)”.

Why there is “mthi $0” instruction, but there is NO emit_move_insn(HI, CONST_0)?
Is such mismatch a bug? If not, why? 

>  In theory it's possible
> to emit the mthi instruction explicitly here though, but we'll need to
> clear something NOT in need_zeroed_hardregs for MIPS anyway (see below).

One question here,  is there situation when only HI is cleared but LO is not cleared?
> 
>>> Here too I think we should just do:
>>> 
>>>      zeroed_hardregs |= reg_class_contents[ST_REGS] & accessible_reg_set;
>>> 
>>> to include all available FCC registers.
>> 
>> What’s the relationship between “ST_REGs” and FCC? (sorry for the stupid question since I am not familiar with the MIPS register set).
> 
> MIPS instruction manual names the 8 one-bit floating condition codes
> FCC0, ..., FCC7, but GCC MIPS backend code names the condition codes
> ST_REG0, ..., ST_REG7.  Maybe it's better to always use the name
> "ST_REG" instead of "FCC" then.
Okay, I see.  So, each ST_REGi register is a 1-bit pseudo register? But physically each of them is 1-bit in a physical register?
> 
>> From the above code, looks like that when any  “ST_REGs” is in “need_zeroed_hardregs”,FCC need to be cleared? 
> 
> Because there is no elegant way to clear one specific FCC bit in MIPS. 
> A "ctc1 $0, $25" instruction will zero them altogether.  If we really
> need to clear only one of them (let's say ST_REG3), we'll have to emit
> something like
> 
> mtc1  $0, $0           # zero FPR0 to ensure it won't contain sNaN
> c.f.s $3, $0, $0
> 
> Then we'll still need to clobber FPR0 with zero.  So anyway we'll have
> to clear some registers not specified in need_zeroed_hardregs.

So, “c.f.s” instruction can be used to clear ONLY one specific FCC bit? 
But you have to clear one FPR (floating pointer register?) first to avoid raising exception? 
My question here is:  is there a case when only FCC need to be cleared but no FPR need to be cleared? 

If NOT, then we can always pick one FPRi  before c.f.s to avoid the issue you mentioned (We’ll have to clear some registers not specified in need_zeroed_hardregs).
> 
> And the question is: is it really allowed to return something other than
> a subset of need_zeroed_hardregs for a TARGET_ZERO_CALL_USED_REGS hook?

Although currently there is no assertion added to force this requirement, I still think that we should keep it.

The “need_zeroed_hardregs” is computed based on 

1. User’s request from command line option;
2. Data flow info of the routine;
3. Abi info of the target;

If zero_call_used_regs target hook return registers out of “need_zeroed_hardregs” set, then it might out of the user’s exception, it should be considered as a bug, I think.

Qing
> If yes then we'll happily to do so (like how the v2 of the patch does),
> otherwise we'd need to clobber those registers NOT in
> need_zeroed_hardregs explicitly.
> -- 
> Xi Ruoyao <xry111@mengyan1223.wang>
> School of Aerospace Science and Technology, Xidian University
  

On Fri, 2022-03-11 at 16:08 +0000, Qing Zhao wrote:

> Why there is “mthi $0” instruction, but there is NO emit_move_insn(HI, CONST_0)?
> Is such mismatch a bug? If not, why? 
> 
> >  In theory it's possible
> > to emit the mthi instruction explicitly here though, but we'll need to
> > clear something NOT in need_zeroed_hardregs for MIPS anyway (see below).
> 
> One question here,  is there situation when only HI is cleared but LO is not cleared?

No, if I interpret the document of -fzero_call_used_regs and
attribute((zero_call_used_regs(...))) correctly.  A 2-reg multiplication
(or division) always set the value of both HI and LO.  Richard has added
a comment for this in mips.cc:

> 12868       /* After a multiplication or division, clobbering HI makes
>     1          the value of LO unpredictable, and vice versa.  This means
>     2          that, for all interesting cases, HI and LO are effectively
>     3          a single register.
>     4 
>     5          We model this by requiring that any value that uses HI
>     6          also uses LO.  */

This is also why the handling of emit_move_insn(HI, CONST_0) was
removed, I guess (the removal happened in the same commit adding this
comment).


> > > 
> Okay, I see.  So, each ST_REGi register is a 1-bit pseudo register?
> But physically each of them is 1-bit in a physical register?

Yes.

> > 
> > Because there is no elegant way to clear one specific FCC bit in MIPS. 
> > A "ctc1 $0, $25" instruction will zero them altogether.  If we really
> > need to clear only one of them (let's say ST_REG3), we'll have to emit
> > something like
> > 
> > mtc1  $0, $0           # zero FPR0 to ensure it won't contain sNaN
> > c.f.s $3, $0, $0
> > 
> > Then we'll still need to clobber FPR0 with zero.  So anyway we'll have
> > to clear some registers not specified in need_zeroed_hardregs.
> 
> So, “c.f.s” instruction can be used to clear ONLY one specific FCC bit? 
> But you have to clear one FPR (floating pointer register?) first to avoid raising exception? 
> My question here is:  is there a case when only FCC need to be cleared but no FPR need to be cleared? 

Yes, for example:

double a, b;

struct x
{
  double a, b;
};

struct x
f(void)
{
  struct x x =
    {
      .a = a,
      .b = b
    };
  if (a < b)
    x.a = x.b;
  return x;
}

It does not need to zero the two FPRs, as they contain the return value.
But a FCC bit needs to be cleared.

> If NOT, then we can always pick one FPRi  before c.f.s to avoid the
> issue you mentioned (We’ll have to clear some registers not specified
> in need_zeroed_hardregs).

I'm now thinking: is there always at least one *GPR* which need to be
cleared?  If it's true, let's say GPR $12, and fcc0 & fcc2 needs to be
cleared, we can use something like:

cfc1 $12, $25
andi $25, 5
ctc1 $12, $25
move $12, $0

> > And the question is: is it really allowed to return something other than
> > a subset of need_zeroed_hardregs for a TARGET_ZERO_CALL_USED_REGS hook?
> 
> Although currently there is no assertion added to force this
> requirement, I still think that we should keep it.
> 
> The “need_zeroed_hardregs” is computed based on 
> 
> 1. User’s request from command line option;
> 2. Data flow info of the routine;
> 3. Abi info of the target;
> 
> If zero_call_used_regs target hook return registers out of
> “need_zeroed_hardregs” set, then it might out of the user’s exception,
> it should be considered as a bug, I think.

I have the same concern.  But now I'm too sleepy... Will try to improve
this tomorrow.
  

On Sat, 2022-03-12 at 01:29 +0800, Xi Ruoyao via Gcc-patches wrote:

> I'm now thinking: is there always at least one *GPR* which need to be
> cleared?  If it's true, let's say GPR $12, and fcc0 & fcc2 needs to be
> cleared, we can use something like:
> 
> cfc1 $12, $25
> andi $25, 5

$12, 5.

I can't type.

> ctc1 $12, $25
> move $12, $0
  

Hi, Ruoyao,

(I might not be able to reply to this thread till next Wed due to a short vacation).

First, some comments on opening bugs against Gcc:

I took a look at the bug reports PR104817 and PR104820:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104820
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104817

I didn’t see a testing case and a script to repeat the error, so I cannot repeat the error at my side.
So, in order for other people to help to study the bug, you need to provide a testing case and
A script or detailed description on how to repeat the bug. 

In addition to that, it will be helpful if you can provide more details on what’ the root cause of the
Issue from your study into the comment part of the bug report. 


> On Mar 11, 2022, at 11:29 AM, Xi Ruoyao <xry111@mengyan1223.wang> wrote:
> 
> On Fri, 2022-03-11 at 16:08 +0000, Qing Zhao wrote:
> 
>> Why there is “mthi $0” instruction, but there is NO emit_move_insn(HI, CONST_0)?
>> Is such mismatch a bug? If not, why? 
>> 
>>>  In theory it's possible
>>> to emit the mthi instruction explicitly here though, but we'll need to
>>> clear something NOT in need_zeroed_hardregs for MIPS anyway (see below).
>> 
>> One question here,  is there situation when only HI is cleared but LO is not cleared?
> 
> No, if I interpret the document of -fzero_call_used_regs and
> attribute((zero_call_used_regs(...))) correctly.  A 2-reg multiplication
> (or division) always set the value of both HI and LO.  Richard has added
> a comment for this in mips.cc:
> 
>> 12868       /* After a multiplication or division, clobbering HI makes
>>    1          the value of LO unpredictable, and vice versa.  This means
>>    2          that, for all interesting cases, HI and LO are effectively
>>    3          a single register.
>>    4 
>>    5          We model this by requiring that any value that uses HI
>>    6          also uses LO.  */
> 
> This is also why the handling of emit_move_insn(HI, CONST_0) was
> removed, I guess (the removal happened in the same commit adding this
> comment).

Okay. I see. 
Then the current handling for HI_REGNUM is reasonable. I suggest to add one 
assertion inside the handling of HI_REGNUM with proper comment:

gcc_assertion (TEST_HARD_REG_BIT (need_zeroed_hardregs, LO_REGNUM));

to catch any unexpected bug. 

Richard, what’s your opinion on this?

> 
> 
>>>> 
>> Okay, I see.  So, each ST_REGi register is a 1-bit pseudo register?
>> But physically each of them is 1-bit in a physical register?
> 
> Yes.
> 
>>> 
>>> Because there is no elegant way to clear one specific FCC bit in MIPS. 
>>> A "ctc1 $0, $25" instruction will zero them altogether.  If we really
>>> need to clear only one of them (let's say ST_REG3), we'll have to emit
>>> something like
>>> 
>>> mtc1  $0, $0           # zero FPR0 to ensure it won't contain sNaN
>>> c.f.s $3, $0, $0
>>> 
>>> Then we'll still need to clobber FPR0 with zero.  So anyway we'll have
>>> to clear some registers not specified in need_zeroed_hardregs.
>> 
>> So, “c.f.s” instruction can be used to clear ONLY one specific FCC bit? 
>> But you have to clear one FPR (floating pointer register?) first to avoid raising exception? 
>> My question here is:  is there a case when only FCC need to be cleared but no FPR need to be cleared? 
> 
> Yes, for example:
> 
> double a, b;
> 
> struct x
> {
>  double a, b;
> };
> 
> struct x
> f(void)
> {
>  struct x x =
>    {
>      .a = a,
>      .b = b
>    };
>  if (a < b)
>    x.a = x.b;
>  return x;
> }
> 
> It does not need to zero the two FPRs, as they contain the return value.
> But a FCC bit needs to be cleared.
Okay.

> 
>> If NOT, then we can always pick one FPRi  before c.f.s to avoid the
>> issue you mentioned (We’ll have to clear some registers not specified
>> in need_zeroed_hardregs).
> 
> I'm now thinking: is there always at least one *GPR* which need to be
> cleared?  If it's true, let's say GPR $12, and fcc0 & fcc2 needs to be
> cleared, we can use something like:

So, you mean, in order to set one FCC bit to zero, we have to set another GPR or FPR to zero first? Otherwise an error might occur? 
Why? (This is unreasonable to me) do I miss anything here?

Qing
> cfc1 $12, $25
> andi $25, 5
> ctc1 $12, $25
> move $12, $0
> 
>>> And the question is: is it really allowed to return something other than
>>> a subset of need_zeroed_hardregs for a TARGET_ZERO_CALL_USED_REGS hook?
>> 
>> Although currently there is no assertion added to force this
>> requirement, I still think that we should keep it.
>> 
>> The “need_zeroed_hardregs” is computed based on 
>> 
>> 1. User’s request from command line option;
>> 2. Data flow info of the routine;
>> 3. Abi info of the target;
>> 
>> If zero_call_used_regs target hook return registers out of
>> “need_zeroed_hardregs” set, then it might out of the user’s exception,
>> it should be considered as a bug, I think.
> 
> I have the same concern.  But now I'm too sleepy... Will try to improve
> this tomorrow.
> -- 
> Xi Ruoyao <xry111@mengyan1223.wang>
> School of Aerospace Science and Technology, Xidian University
  

On Fri, 2022-03-11 at 21:26 +0000, Qing Zhao wrote:
> Hi, Ruoyao,
> 
> (I might not be able to reply to this thread till next Wed due to a
> short vacation).
> 
> First, some comments on opening bugs against Gcc:
> 
> I took a look at the bug reports PR104817 and PR104820:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104820
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104817
> 
> I didn’t see a testing case and a script to repeat the error, so I
> cannot repeat the error at my side.

I've put the test case, but maybe you didn't see it because it is too
simple:

echo 'int t() {}' | /home/xry111/git-repos/gcc-test-mips/gcc/cc1 -nostdinc -fzero-call-used-regs=all

An empty function is enough to break -fzero-call-used-regs=all.  And if
you append -mips64r2 to the cc1 command line you'll get 104820.  I
enabled 4 existing tests for MIPS (reported "not work" on MIPS) in the
patch so I think it's unnecessary to add new test cases.

Richard: can we use MIPS_EPILOGUE_TEMP as a scratch register in the
sequence for zeroing the call-used registers, and then zero itself
(despite it's not in need_zeroed_hardregs)?
  

On Sat, 2022-03-12 at 18:48 +0800, Xi Ruoyao via Gcc-patches wrote:
> On Fri, 2022-03-11 at 21:26 +0000, Qing Zhao wrote:
> > Hi, Ruoyao,
> > 
> > (I might not be able to reply to this thread till next Wed due to a
> > short vacation).
> > 
> > First, some comments on opening bugs against Gcc:
> > 
> > I took a look at the bug reports PR104817 and PR104820:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104820
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104817
> > 
> > I didn’t see a testing case and a script to repeat the error, so I
> > cannot repeat the error at my side.
> 
> I've put the test case, but maybe you didn't see it because it is too
> simple:
> 
> echo 'int t() {}' | /home/xry111/git-repos/gcc-test-mips/gcc/cc1 -
> nostdinc -fzero-call-used-regs=all
> 
> An empty function is enough to break -fzero-call-used-regs=all.  And
> if
> you append -mips64r2 to the cc1 command line you'll get 104820.  I
> enabled 4 existing tests for MIPS (reported "not work" on MIPS) in the
> patch so I think it's unnecessary to add new test cases.
> 
> Richard: can we use MIPS_EPILOGUE_TEMP as a scratch register in the
> sequence for zeroing the call-used registers, and then zero itself
> (despite it's not in need_zeroed_hardregs)?

No, it leads to an ICE at stage 3 bootstrapping :(.

Now I think the only rational ways are:

(1) allow zeroing more registers than need_zeroed_hardregs.

Or

(2) allow zeroing less registers than need_zeroed_hardregs (then I'll
skip ST_REGS, after all they are just 8 bits in total).

If all these are unacceptable, then

(3) I'll just call sorry in MIPS target hook to tell not to use this
feature on MIPS.
  

Sorry for the slow response, was out for a few days.

Xi Ruoyao <xry111@mengyan1223.wang> writes:
> On Sat, 2022-03-12 at 18:48 +0800, Xi Ruoyao via Gcc-patches wrote:
>> On Fri, 2022-03-11 at 21:26 +0000, Qing Zhao wrote:
>> > Hi, Ruoyao,
>> > 
>> > (I might not be able to reply to this thread till next Wed due to a
>> > short vacation).
>> > 
>> > First, some comments on opening bugs against Gcc:
>> > 
>> > I took a look at the bug reports PR104817 and PR104820:
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104820
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104817
>> > 
>> > I didn’t see a testing case and a script to repeat the error, so I
>> > cannot repeat the error at my side.
>> 
>> I've put the test case, but maybe you didn't see it because it is too
>> simple:
>> 
>> echo 'int t() {}' | /home/xry111/git-repos/gcc-test-mips/gcc/cc1 -
>> nostdinc -fzero-call-used-regs=all
>> 
>> An empty function is enough to break -fzero-call-used-regs=all.  And
>> if
>> you append -mips64r2 to the cc1 command line you'll get 104820.  I
>> enabled 4 existing tests for MIPS (reported "not work" on MIPS) in the
>> patch so I think it's unnecessary to add new test cases.
>> 
>> Richard: can we use MIPS_EPILOGUE_TEMP as a scratch register in the
>> sequence for zeroing the call-used registers, and then zero itself
>> (despite it's not in need_zeroed_hardregs)?
>
> No, it leads to an ICE at stage 3 bootstrapping :(.
>
> Now I think the only rational ways are:
>
> (1) allow zeroing more registers than need_zeroed_hardregs.

I think this is the way to go.  I agree it's a bit hacky, but it seems
like the least worst option.

A less hacky alternative would be to pass an extra argument to the hook
that contains the set of registers that the hook is *allowed* to clobber.
For -fzero-call-used-regs=X, this new argument would be the set that
would have been chosen for -fzero-call-used-regs=all, regardless of
what X actually is.  We could then assert that the extra registers we
want to clobber are in that set (which will be true for all values of X).

> Or
>
> (2) allow zeroing less registers than need_zeroed_hardregs (then I'll
> skip ST_REGS, after all they are just 8 bits in total).

Yeah, this is explicitly OK, provided that the target maintainers
feel that the contents of the registers in question are not a significant
security concern.  I don't feel I can make that call though.  It's really
a question for the userbase.

Thanks,
Richard

> If all these are unacceptable, then
>
> (3) I'll just call sorry in MIPS target hook to tell not to use this
> feature on MIPS.
  

On Mon, 2022-03-14 at 16:04 +0000, Richard Sandiford wrote:

> Xi Ruoyao <xry111@mengyan1223.wang> writes:

> > Now I think the only rational ways are:
> > 
> > (1) allow zeroing more registers than need_zeroed_hardregs.
> 
> I think this is the way to go.  I agree it's a bit hacky, but it seems
> like the least worst option.

> > (2) allow zeroing less registers than need_zeroed_hardregs (then I'll
> > skip ST_REGS, after all they are just 8 bits in total).
> 
> Yeah, this is explicitly OK, provided that the target maintainers
> feel that the contents of the registers in question are not a significant
> security concern.  I don't feel I can make that call though.  It's really
> a question for the userbase.

To me, I believe nobody will write some security-critical code depending
on a floating-point compare result :).  Maybe I'm wrong by mixing up the
concept of "security" and "safety".
  

> On Mar 14, 2022, at 11:04 AM, Richard Sandiford <richard.sandiford@arm.com> wrote:
> 
> Sorry for the slow response, was out for a few days.
> 
> Xi Ruoyao <xry111@mengyan1223.wang> writes:
>> On Sat, 2022-03-12 at 18:48 +0800, Xi Ruoyao via Gcc-patches wrote:
>>> On Fri, 2022-03-11 at 21:26 +0000, Qing Zhao wrote:
>>>> Hi, Ruoyao,
>>>> 
>>>> (I might not be able to reply to this thread till next Wed due to a
>>>> short vacation).
>>>> 
>>>> First, some comments on opening bugs against Gcc:
>>>> 
>>>> I took a look at the bug reports PR104817 and PR104820:
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104820
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104817
>>>> 
>>>> I didn’t see a testing case and a script to repeat the error, so I
>>>> cannot repeat the error at my side.
>>> 
>>> I've put the test case, but maybe you didn't see it because it is too
>>> simple:
>>> 
>>> echo 'int t() {}' | /home/xry111/git-repos/gcc-test-mips/gcc/cc1 -
>>> nostdinc -fzero-call-used-regs=all
>>> 
>>> An empty function is enough to break -fzero-call-used-regs=all.  And
>>> if
>>> you append -mips64r2 to the cc1 command line you'll get 104820.  I
>>> enabled 4 existing tests for MIPS (reported "not work" on MIPS) in the
>>> patch so I think it's unnecessary to add new test cases.
>>> 
>>> Richard: can we use MIPS_EPILOGUE_TEMP as a scratch register in the
>>> sequence for zeroing the call-used registers, and then zero itself
>>> (despite it's not in need_zeroed_hardregs)?
>> 
>> No, it leads to an ICE at stage 3 bootstrapping :(.
>> 
>> Now I think the only rational ways are:
>> 
>> (1) allow zeroing more registers than need_zeroed_hardregs.
> 
> I think this is the way to go.  I agree it's a bit hacky, but it seems
> like the least worst option.
> 
> A less hacky alternative would be to pass an extra argument to the hook
> that contains the set of registers that the hook is *allowed* to clobber.
> For -fzero-call-used-regs=X, this new argument would be the set that
> would have been chosen for -fzero-call-used-regs=all, regardless of
> what X actually is.  We could then assert that the extra registers we
> want to clobber are in that set (which will be true for all values of X).

If we have to go this way, I think it’s better to make the change you suggested above, 
and then also update the documentation, both internal documentation on how to define
 the hook and the user level documentation on what the user might expect when using 
this option (i.e, it’s possible that the compiler might clear more registers than the user 
requests on some targets due to the implementation limitation). 

I can make this change if we decide to do this.

Thanks.

Qing

> 
>> Or
>> 
>> (2) allow zeroing less registers than need_zeroed_hardregs (then I'll
>> skip ST_REGS, after all they are just 8 bits in total).
> 
> Yeah, this is explicitly OK, provided that the target maintainers
> feel that the contents of the registers in question are not a significant
> security concern.  I don't feel I can make that call though.  It's really
> a question for the userbase.
> 
> Thanks,
> Richard
> 
>> If all these are unacceptable, then
>> 
>> (3) I'll just call sorry in MIPS target hook to tell not to use this
>> feature on MIPS.
  

> 
> If we have to go this way, I think it’s better to make the change you
> suggested above, 
> and then also update the documentation, both internal documentation on
> how to define
>  the hook and the user level documentation on what the user might
> expect when using 
> this option (i.e, it’s possible that the compiler might clear more
> registers than the user 
> requests on some targets due to the implementation limitation). 
> 
> I can make this change if we decide to do this.

I'd vote for this.  Richard?
  

Xi Ruoyao <xry111@mengyan1223.wang> writes:
>> 
>> If we have to go this way, I think it’s better to make the change you
>> suggested above, 
>> and then also update the documentation, both internal documentation on
>> how to define
>>  the hook and the user level documentation on what the user might
>> expect when using 
>> this option (i.e, it’s possible that the compiler might clear more
>> registers than the user 
>> requests on some targets due to the implementation limitation). 
>> 
>> I can make this change if we decide to do this.
>
> I'd vote for this.  Richard?

Fine by me too, although I don't think this should be mentioned
in the user documentation.  E.g. used-arg means that non-argument,
non-return registers can have any value on return from the function;
the compiler doesn't make any guarantees.  If the compiler happens to
use a temporary register in the implementation of the option, and if
that temporary register happens to still be zero on return, then
that's OK.  It's just an internal implementation detail.  The same
thing could happen for any part of the epilogue.

Thanks,
Richard
  

> On Mar 18, 2022, at 11:09 AM, Richard Sandiford <richard.sandiford@arm.com> wrote:
> 
> Xi Ruoyao <xry111@mengyan1223.wang> writes:
>>> 
>>> If we have to go this way, I think it’s better to make the change you
>>> suggested above, 
>>> and then also update the documentation, both internal documentation on
>>> how to define
>>>  the hook and the user level documentation on what the user might
>>> expect when using 
>>> this option (i.e, it’s possible that the compiler might clear more
>>> registers than the user 
>>> requests on some targets due to the implementation limitation). 
>>> 
>>> I can make this change if we decide to do this.
>> 
>> I'd vote for this.  Richard?
> 
> Fine by me too, although I don't think this should be mentioned
> in the user documentation.  E.g. used-arg means that non-argument,
> non-return registers can have any value on return from the function;
> the compiler doesn't make any guarantees.  If the compiler happens to
> use a temporary register in the implementation of the option, and if
> that temporary register happens to still be zero on return, then
> that's OK.  It's just an internal implementation detail.  The same
> thing could happen for any part of the epilogue.

This makes good sense to me. I agree. 

Okay, will just add an extra argument and update the internal documentation. 

thanks.

Qing
> 
> Thanks,
> Richard
  

On Sat, 12 Mar 2022, Xi Ruoyao via Gcc-patches wrote:

> I'm now thinking: is there always at least one *GPR* which need to be
> cleared?  If it's true, let's say GPR $12, and fcc0 & fcc2 needs to be
> cleared, we can use something like:
> 
> cfc1 $12, $25
> andi $25, 5
> ctc1 $12, $25
> move $12, $0

 There's always $1 ($at) available and we're in a function's epilogue, so 
there should be plenty of dead temporaries available as well.  For legacy 
ISAs you'd need to use the FCSR instead ($31) and two temporaries would be 
required as the condition code bits are located in the upper half.

 FWIW,

  Maciej