From: "Kewen.Lin" <linkw@linux.ibm.com>
To: HAO CHEN GUI <guihaoc@linux.ibm.com>
Cc: Segher Boessenkool <segher@kernel.crashing.org>,
David <dje.gcc@gmail.com>, Peter Bergner <bergner@linux.ibm.com>,
gcc-patches <gcc-patches@gcc.gnu.org>,
Michael Meissner <meissner@linux.ibm.com>
Subject: Re: [patch-1, rs6000] enable fctiw on old archs [PR112707]
Date: Tue, 5 Dec 2023 15:48:11 +0800 [thread overview]
Message-ID: <41b6b8ae-df3f-4a96-bda8-9f99c39572ce@linux.ibm.com> (raw)
In-Reply-To: <770bdc23-24cc-4699-af13-38eab3f32b80@linux.ibm.com>
Hi Haochen,
on 2023/12/1 10:41, HAO CHEN GUI wrote:
> Hi,
> SImode in float register is supported on P7 above. It causes "fctiw"
> can be generated on old 32-bit processors as the output operand of
typo? I guess you meant to say "can NOT"?
> fctiw insn is a SImode in float/double register. This patch fixes the
> problem by adding an expand and an insn pattern for fctiw. The output
> of new pattern is SFmode. When the target doesn't support SImode in
> float register, it calls the new pattern and convert the SFmode to
> SImode via stack.
Assuming that due to the inconsistent ISA support levels between stfiwx
and lfiw[az]x, it's not practical to support SImode in FP regs.
>
> Bootstrapped and tested on x86 and powerpc64-linux BE and LE with
> no regressions. Is this OK for trunk?
>
> Thanks
> Gui Haochen
>
> ChangeLog
> rs6000: enable fctiw on old archs
>
> The powerpc 32-bit processors (e.g. 5470) supports "fctiw" instruction,
> but the instruction can't be generated on such platforms as the insn is
> guard by TARGET_POPCNTD. The root cause is SImode in float register is
> supported from Power7. Actually implementation of "fctiw" only needs
> stfiwx which is supported by the old 320-bit processors. This patch
> enables "fctiw" expand for these processors.
>
> gcc/
> PR target/112707
> * config/rs6000/rs6000.md (UNSPEC_STFIWX_SF, UNSPEC_FCTIW_SF): New.
> (expand lrint<mode>si2): New.
> (insn lrint<mode>si2): Rename to...
> (lrint<mode>si_internal): ...this, and remove guard TARGET_POPCNTD.
> (lrint<mode>si_internal2): New.
> (stfiwx_sf): New.
>
> gcc/testsuite/
> PR target/112707
> * gcc.target/powerpc/pr112707-1.c: New.
>
> patch.diff
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index d4337ce42a9..1b207522ad5 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -90,6 +90,7 @@ (define_c_enum "unspec"
> UNSPEC_TLSTLS_PCREL
> UNSPEC_FIX_TRUNC_TF ; fadd, rounding towards zero
> UNSPEC_STFIWX
> + UNSPEC_STFIWX_SF
> UNSPEC_POPCNTB
> UNSPEC_FRES
> UNSPEC_SP_SET
> @@ -111,6 +112,7 @@ (define_c_enum "unspec"
> UNSPEC_PARITY
> UNSPEC_CMPB
> UNSPEC_FCTIW
> + UNSPEC_FCTIW_SF
> UNSPEC_FCTID
> UNSPEC_LFIWAX
> UNSPEC_LFIWZX
> @@ -6722,11 +6724,39 @@ (define_insn "lrint<mode>di2"
> "fctid %0,%1"
> [(set_attr "type" "fp")])
>
> -(define_insn "lrint<mode>si2"
> +(define_expand "lrint<mode>si2"
> [(set (match_operand:SI 0 "gpc_reg_operand" "=d")
> (unspec:SI [(match_operand:SFDF 1 "gpc_reg_operand" "<rreg2>")]
> UNSPEC_FCTIW))]
> - "TARGET_HARD_FLOAT && TARGET_POPCNTD"
> + "TARGET_HARD_FLOAT && TARGET_STFIWX"
> +{
> + /* For those old archs in which SImode can't be hold in float registers,
> + call lrint<mode>si_internal2 to put the result in SFmode then
> + convert it via stack. */
> + if (!TARGET_POPCNTD)
> + {
> + rtx tmp = gen_reg_rtx (SFmode);
> + emit_insn (gen_lrint<mode>si_internal2 (tmp, operands[1]));
Considering some existing supports eg: "fix_trunc<mode>si2_stfiwx" adopting
DImode, I think we can do the similar thing here, ie:
rtx tmp = gen_reg_rtx (DImode);
emit_insn (gen_lrint<mode>si_di (tmp, operands[1]));
> + rtx stack = rs6000_allocate_stack_temp (SImode, false, true);
> + emit_insn (gen_stfiwx_sf (stack, tmp));
...
emit_insn (gen_stfiwx (stack, tmp));
Theoretically even if !TARGET_STFIWX, we can still save the fpr into
memory and load the appropriate 4 bytes from that, but TARGET_STFIWX (PPC)
is quite old already, introducing such complexity here seems not worthy.
> + emit_move_insn (operands[0], stack);
> + DONE;
> + }
> +})
> +
> +(define_insn "lrint<mode>si_internal"
Nit: This can be unnamed, maybe something like "*lrint<mode>si"?
> + [(set (match_operand:SI 0 "gpc_reg_operand" "=d")
> + (unspec:SI [(match_operand:SFDF 1 "gpc_reg_operand" "<rreg2>")]
> + UNSPEC_FCTIW))]
> + "TARGET_HARD_FLOAT"
Nit: Add "&& TARGET_POPCNTD" to the condition.
> + "fctiw %0,%1"
> + [(set_attr "type" "fp")])
> +
> +(define_insn "lrint<mode>si_internal2"
can be: lrint<mode>si_di
> + [(set (match_operand:SF 0 "gpc_reg_operand" "=d")
> + (unspec:SF [(match_operand:SFDF 1 "gpc_reg_operand" "<rreg2>")]
> + UNSPEC_FCTIW_SF))]
Use DI to replace SF here and just still use UNSPEC_FCTIW.
> + "TARGET_HARD_FLOAT"
Add "&& !TARGET_POPCNTD" to the condition.
> "fctiw %0,%1"
> [(set_attr "type" "fp")])
>
> @@ -6801,6 +6831,14 @@ (define_insn "stfiwx"
> [(set_attr "type" "fpstore")
> (set_attr "isa" "*,p8v")])
>
> +(define_insn "stfiwx_sf"
> + [(set (match_operand:SI 0 "memory_operand" "=Z")
> + (unspec:SI [(match_operand:SF 1 "gpc_reg_operand" "d")]
> + UNSPEC_STFIWX_SF))]
> + "TARGET_STFIWX"
> + "stfiwx %1,%y0"
> + [(set_attr "type" "fpstore")])
Then this part isn't needed.
> +
> ;; If we don't have a direct conversion to single precision, don't enable this
> ;; conversion for 32-bit without fast math, because we don't have the insn to
> ;; generate the fixup swizzle to avoid double rounding problems.
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr112707-1.c b/gcc/testsuite/gcc.target/powerpc/pr112707-1.c
> new file mode 100644
> index 00000000000..32f708c5402
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr112707-1.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile { target { powerpc*-*-* && be } } } */
> +/* { dg-options "-O2 -mdejagnu-cpu=7450 -m32 -fno-math-errno" } */
> +/* { dg-require-effective-target ilp32 } */
Nit: "powerpc*-*-* && be" and "-m32" are redundant as pointed out in the review
of patch-2.
BR,
Kewen
> +/* { dg-final { scan-assembler-times {\mfctiw\M} 2 } } */
> +/* { dg-final { scan-assembler-times {\mstfiwx\M} 2 } } */
> +
> +int test1 (double a)
> +{
> + return __builtin_irint (a);
> +}
> +
> +int test2 (float a)
> +{
> + return __builtin_irint (a);
> +}
>
prev parent reply other threads:[~2023-12-05 7:48 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-01 2:41 HAO CHEN GUI
2023-12-05 7:48 ` Kewen.Lin [this message]
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=41b6b8ae-df3f-4a96-bda8-9f99c39572ce@linux.ibm.com \
--to=linkw@linux.ibm.com \
--cc=bergner@linux.ibm.com \
--cc=dje.gcc@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=guihaoc@linux.ibm.com \
--cc=meissner@linux.ibm.com \
--cc=segher@kernel.crashing.org \
/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).