From: Qing Zhao <qing.zhao@oracle.com>
To: Richard Sandiford <richard.sandiford@arm.com>
Cc: Xi Ruoyao <xry111@mengyan1223.wang>,
YunQiang Su <yunqiang.su@cipunited.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
Jakub Jelinek <jakub@gcc.gnu.org>, Jeff Law <law@redhat.com>
Subject: Re: [PATCH RFC] mips: add TARGET_ZERO_CALL_USED_REGS hook [PR104817, PR104820]
Date: Thu, 10 Mar 2022 20:31:13 +0000 [thread overview]
Message-ID: <53408AB1-0FA4-4529-95D9-A10FA8999DF6@oracle.com> (raw)
In-Reply-To: <mpt35jrc7ac.fsf@arm.com>
> 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"
next prev parent reply other threads:[~2022-03-10 20:31 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-07 20:40 Xi Ruoyao
2022-03-09 18:25 ` Richard Sandiford
2022-03-10 11:53 ` Xi Ruoyao
2022-03-10 13:41 ` [PATCH v2 " Xi Ruoyao
2022-03-10 13:45 ` [PATCH v2 RFC, resend] " Xi Ruoyao
2022-03-10 20:31 ` Qing Zhao [this message]
2022-03-11 2:54 ` [PATCH RFC] " Xi Ruoyao
2022-03-11 16:08 ` Qing Zhao
2022-03-11 17:29 ` Xi Ruoyao
2022-03-11 17:31 ` Xi Ruoyao
2022-03-11 21:26 ` Qing Zhao
2022-03-12 10:48 ` Xi Ruoyao
2022-03-13 6:03 ` Xi Ruoyao
2022-03-14 16:04 ` Richard Sandiford
2022-03-14 17:05 ` Xi Ruoyao
2022-03-16 20:27 ` Qing Zhao
2022-03-18 13:11 ` Xi Ruoyao
2022-03-18 16:09 ` Richard Sandiford
2022-03-18 18:51 ` Qing Zhao
2022-04-01 14:32 ` Maciej W. Rozycki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53408AB1-0FA4-4529-95D9-A10FA8999DF6@oracle.com \
--to=qing.zhao@oracle.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@gcc.gnu.org \
--cc=law@redhat.com \
--cc=richard.sandiford@arm.com \
--cc=xry111@mengyan1223.wang \
--cc=yunqiang.su@cipunited.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).