On Mon, 3 Mar 2025 at 12:29, Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Tamar Christina <tamar.christ...@arm.com> writes:
> >> -----Original Message-----
> >> From: Richard Sandiford <richard.sandif...@arm.com>
> >> Sent: Monday, March 3, 2025 10:12 AM
> >> To: Tamar Christina <tamar.christ...@arm.com>
> >> Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; Richard Earnshaw
> >> <richard.earns...@arm.com>; ktkac...@gcc.gnu.org
> >> Subject: Re: [PATCH]AArch64: force operand to fresh register to avoid
> >> subreg
> >> issues [PR118892]
> >>
> >> Tamar Christina <tamar.christ...@arm.com> writes:
> >> > Hi All,
> >> >
> >> > When the input is already a subreg and we try to make a paradoxical
> >> > subreg out of it for copysign this can fail if it violates the sugreg
> >>
> >> subreg
> >>
> >> > relationship.
> >> >
> >> > Use force_lowpart_subreg instead of lowpart_subreg to then force the
> >> > results to a register instead of ICEing.
> >> >
> >> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> >> >
> >> > Ok for master?
> >> >
> >> > Thanks,
> >> > Tamar
> >> >
> >> > gcc/ChangeLog:
> >> >
> >> > PR target/118892
> >> > * config/aarch64/aarch64.md (copysign<GPF:mode>3): Use
> >> > force_lowpart_subreg instead of lowpart_subreg.
> >> >
> >> > gcc/testsuite/ChangeLog:
> >> >
> >> > PR target/118892
> >> > * gcc.target/aarch64/copysign-pr118892.c: New test.
> >> >
> >> > ---
> >> >
> >> > diff --git a/gcc/config/aarch64/aarch64.md
> >> > b/gcc/config/aarch64/aarch64.md
> >> > index
> >> cfe730f3732ce45c914b30a908851a4a7dd77c0f..62be9713cf417922b3c06e38f
> >> 12f401872751fa2 100644
> >> > --- a/gcc/config/aarch64/aarch64.md
> >> > +++ b/gcc/config/aarch64/aarch64.md
> >> > @@ -7479,8 +7479,8 @@ (define_expand "copysign<GPF:mode>3"
> >> > && real_isneg (CONST_DOUBLE_REAL_VALUE (op2_elt)))
> >> > {
> >> > emit_insn (gen_ior<vq_int_equiv>3 (
> >> > - lowpart_subreg (<VQ_INT_EQUIV>mode, operands[0], <MODE>mode),
> >> > - lowpart_subreg (<VQ_INT_EQUIV>mode, operands[1], <MODE>mode),
> >> > + force_lowpart_subreg (<VQ_INT_EQUIV>mode, operands[0],
> >> <MODE>mode),
> >> > + force_lowpart_subreg (<VQ_INT_EQUIV>mode, operands[1],
> >> <MODE>mode),
> >>
> >> force_lowpart_subreg conditionally forces the SUBREG_REG into a new
> >> temporary
> >> register and then takes the subreg of that. It's therefore only
> >> appropriate
> >> for source operands, not destination operands.
> >>
> >> It's true that the same problem could in principle occur for the
> >> destination, but that would need to be fixed in a different way.
> >>
> >
> > Ah, true. Should have thought about it a bit more.
> >
> >> OK with just the operands[1] change, without the operands[0] change.
> >>
> >
> > I forgot to ask if OK for GCC 14 backport after some stew.
>
> Yeah, ok for GCC 14 too. The force_lowpart_subreg function hasn't been
> backported to GCC 14 yet, but I think it should be (as part of this patch).
> Other backportable fixes rely on it too.
>
Looks like I was too conservative when I backported my fix for PR
114801, and chose not to include force_lowpart_subreg.
Should my backport be updated to match trunk once force_lowpart_subreg
is backported to gcc-14 too?
(see https://gcc.gnu.org/pipermail/gcc-patches/2025-January/673048.html)
Thanks,
Christophe
> Thanks,
> Richard
>
> >
> > Thanks,
> > Tamar
> >> Thanks,
> >> Richard
> >>
> >>
> >> > v_bitmask));
> >> > DONE;
> >> > }
> >> > diff --git a/gcc/testsuite/gcc.target/aarch64/copysign-pr118892.c
> >> b/gcc/testsuite/gcc.target/aarch64/copysign-pr118892.c
> >> > new file mode 100644
> >> > index
> >> 0000000000000000000000000000000000000000..adfa30dc3e2db895af4f205
> >> 7bdd1011fdb7d4537
> >> > --- /dev/null
> >> > +++ b/gcc/testsuite/gcc.target/aarch64/copysign-pr118892.c
> >> > @@ -0,0 +1,11 @@
> >> > +/* { dg-do compile } */
> >> > +/* { dg-options "-Ofast" } */
> >> > +
> >> > +double l();
> >> > +double f()
> >> > +{
> >> > + double t6[2] = {l(), l()};
> >> > + double t7[2];
> >> > + __builtin_memcpy(&t7, &t6, sizeof(t6));
> >> > + return -__builtin_fabs(t7[1]);
> >> > +}
