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

[Xi Ruoyao <xry111@mengyan1223.wang>]

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;
> >  \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?

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.
-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University