Christophe Lyon <christophe.l...@linaro.org> writes:
> 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)
One for Richrd E, but FWIW, either way sounds ok to me.
Thanks,
Richard
