[PATCH] Fix (fcopysign x, NEGATIVE_CONST) -> (fneg (fabs x)) simplification [PR112483]

[PATCH] Fix (fcopysign x, NEGATIVE_CONST) -> (fneg (fabs x)) simplification [PR112483]

[Xi Ruoyao]

(fcopysign x, NEGATIVE_CONST) can be simplified to (fneg (fabs x)), but
a logic error in the code caused it mistakenly simplified to (fneg x)
instead.

gcc/ChangeLog:

	PR rtl-optimization/112483
	* simplify-rtx.cc (simplify_binary_operation_1) <case COPYSIGN>:
	Fix the simplification of (fcopysign x, NEGATIVE_CONST).
---

Bootstrapped and regtested on loongarch64-linux-gnu and
x86_64-linux-gnu.  Ok for trunk?

 gcc/simplify-rtx.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/simplify-rtx.cc b/gcc/simplify-rtx.cc
index 69d87579d9c..2d2e5a3c1ca 100644
--- a/gcc/simplify-rtx.cc
+++ b/gcc/simplify-rtx.cc
@@ -4392,7 +4392,7 @@ simplify_ashift:
 	  real_convert (&f1, mode, CONST_DOUBLE_REAL_VALUE (trueop1));
 	  rtx tmp = simplify_gen_unary (ABS, mode, op0, mode);
 	  if (REAL_VALUE_NEGATIVE (f1))
-	    tmp = simplify_gen_unary (NEG, mode, op0, mode);
+	    tmp = simplify_gen_unary (NEG, mode, tmp, mode);
 	  return tmp;
 	}
       if (GET_CODE (op0) == NEG || GET_CODE (op0) == ABS)
-- 
2.42.1

Re: [PATCH] Fix (fcopysign x, NEGATIVE_CONST) -> (fneg (fabs x)) simplification [PR112483]

[Richard Biener]

On Sun, Nov 12, 2023 at 9:27 PM Xi Ruoyao <xry111@xry111.site> wrote:
>
> (fcopysign x, NEGATIVE_CONST) can be simplified to (fneg (fabs x)), but
> a logic error in the code caused it mistakenly simplified to (fneg x)
> instead.

OK.

> gcc/ChangeLog:
>
>         PR rtl-optimization/112483
>         * simplify-rtx.cc (simplify_binary_operation_1) <case COPYSIGN>:
>         Fix the simplification of (fcopysign x, NEGATIVE_CONST).
> ---
>
> Bootstrapped and regtested on loongarch64-linux-gnu and
> x86_64-linux-gnu.  Ok for trunk?
>
>  gcc/simplify-rtx.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/simplify-rtx.cc b/gcc/simplify-rtx.cc
> index 69d87579d9c..2d2e5a3c1ca 100644
> --- a/gcc/simplify-rtx.cc
> +++ b/gcc/simplify-rtx.cc
> @@ -4392,7 +4392,7 @@ simplify_ashift:
>           real_convert (&f1, mode, CONST_DOUBLE_REAL_VALUE (trueop1));
>           rtx tmp = simplify_gen_unary (ABS, mode, op0, mode);
>           if (REAL_VALUE_NEGATIVE (f1))
> -           tmp = simplify_gen_unary (NEG, mode, op0, mode);
> +           tmp = simplify_gen_unary (NEG, mode, tmp, mode);
>           return tmp;
>         }
>        if (GET_CODE (op0) == NEG || GET_CODE (op0) == ABS)
> --
> 2.42.1
>

RE: [PATCH] Fix (fcopysign x, NEGATIVE_CONST) -> (fneg (fabs x)) simplification [PR112483]

[Tamar Christina]

> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Monday, November 13, 2023 6:55 AM
> To: Xi Ruoyao <xry111@xry111.site>
> Cc: gcc-patches@gcc.gnu.org; chenglulu <chenglulu@loongson.cn>;
> i@xen0n.name; xuchenghua@loongson.cn; Tamar Christina
> <Tamar.Christina@arm.com>; tschwinge@gcc.gnu.org; Roger Sayle
> <roger@nextmovesoftware.com>
> Subject: Re: [PATCH] Fix (fcopysign x, NEGATIVE_CONST) -> (fneg (fabs x))
> simplification [PR112483]
> 
> On Sun, Nov 12, 2023 at 9:27 PM Xi Ruoyao <xry111@xry111.site> wrote:
> >
> > (fcopysign x, NEGATIVE_CONST) can be simplified to (fneg (fabs x)),
> > but a logic error in the code caused it mistakenly simplified to (fneg
> > x) instead.

The fix aside, I actually wonder if simplify-rtx.cc should be doing this at all.
The mid-end didn't do it because the target said it had an optab for the
copysign operation.  Otherwise during expand_COPYSIGN it would have been
expanded as FNEG (FABS (..)) already.

In the case of e.g. longaarch64 It looks like the target actually has an fcopysign
Instruction, so wouldn't this rewriting by simplify-rtx be a de-optimization?

Thanks,
Tamar
> 
> OK.
> 
> > gcc/ChangeLog:
> >
> >         PR rtl-optimization/112483
> >         * simplify-rtx.cc (simplify_binary_operation_1) <case COPYSIGN>:
> >         Fix the simplification of (fcopysign x, NEGATIVE_CONST).
> > ---
> >
> > Bootstrapped and regtested on loongarch64-linux-gnu and
> > x86_64-linux-gnu.  Ok for trunk?
> >
> >  gcc/simplify-rtx.cc | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gcc/simplify-rtx.cc b/gcc/simplify-rtx.cc index
> > 69d87579d9c..2d2e5a3c1ca 100644
> > --- a/gcc/simplify-rtx.cc
> > +++ b/gcc/simplify-rtx.cc
> > @@ -4392,7 +4392,7 @@ simplify_ashift:
> >           real_convert (&f1, mode, CONST_DOUBLE_REAL_VALUE (trueop1));
> >           rtx tmp = simplify_gen_unary (ABS, mode, op0, mode);
> >           if (REAL_VALUE_NEGATIVE (f1))
> > -           tmp = simplify_gen_unary (NEG, mode, op0, mode);
> > +           tmp = simplify_gen_unary (NEG, mode, tmp, mode);
> >           return tmp;
> >         }
> >        if (GET_CODE (op0) == NEG || GET_CODE (op0) == ABS)
> > --
> > 2.42.1
> >

Re: [PATCH] Fix (fcopysign x, NEGATIVE_CONST) -> (fneg (fabs x)) simplification [PR112483]

[Xi Ruoyao]

On Mon, 2023-11-13 at 07:09 +0000, Tamar Christina wrote:
> In the case of e.g. longaarch64 It looks like the target actually has an fcopysign
> Instruction, so wouldn't this rewriting by simplify-rtx be a de-optimization?

Yes it seems a de-optimization on LoongArch.  For this micro-benchmark:

int main()
{
	#pragma GCC unroll(100)
	for (int i = 0; i < 1000000000; i++) {
		float x = -1, a = 1.23456;
		asm volatile ("":"+f"(a));
#ifdef DISALLOW_COPYSIGN_OPTIMIZATION
		asm("":"+f"(x));
#endif
		a = __builtin_copysignf(a, x);
		asm(""::"f"(a));
	}
}

If DISALLOW_COPYSIGN_OPTIMIZATION is defined, the result is faster for
0.23 seconds.

I'll submit another patch to disable this.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Fix (fcopysign x, NEGATIVE_CONST) -> (fneg (fabs x)) simplification [PR112483]

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 2939 bytes --]

On Sun, Nov 12, 2023, 23:10 Tamar Christina <Tamar.Christina@arm.com> wrote:

> > -----Original Message-----
> > From: Richard Biener <richard.guenther@gmail.com>
> > Sent: Monday, November 13, 2023 6:55 AM
> > To: Xi Ruoyao <xry111@xry111.site>
> > Cc: gcc-patches@gcc.gnu.org; chenglulu <chenglulu@loongson.cn>;
> > i@xen0n.name; xuchenghua@loongson.cn; Tamar Christina
> > <Tamar.Christina@arm.com>; tschwinge@gcc.gnu.org; Roger Sayle
> > <roger@nextmovesoftware.com>
> > Subject: Re: [PATCH] Fix (fcopysign x, NEGATIVE_CONST) -> (fneg (fabs x))
> > simplification [PR112483]
> >
> > On Sun, Nov 12, 2023 at 9:27 PM Xi Ruoyao <xry111@xry111.site> wrote:
> > >
> > > (fcopysign x, NEGATIVE_CONST) can be simplified to (fneg (fabs x)),
> > > but a logic error in the code caused it mistakenly simplified to (fneg
> > > x) instead.
>
> The fix aside, I actually wonder if simplify-rtx.cc should be doing this
> at all.
> The mid-end didn't do it because the target said it had an optab for the
> copysign operation.  Otherwise during expand_COPYSIGN it would have been
> expanded as FNEG (FABS (..)) already.
>
> In the case of e.g. longaarch64 It looks like the target actually has an
> fcopysign
> Instruction, so wouldn't this rewriting by simplify-rtx be a
> de-optimization?
>

Maybe the simplify_gen_unary under the if statement should really
be simplify_unary_operation.
This allows for constant folding but not generating a non canonical form
here.
Also note Canonical RTL forms have a section in the internals document too.
The gimple level Canonical forms are not documented yet; I started writing
some of it on the wiki though: https://gcc.gnu.org/wiki/GimpleCanonical .

Thanks,
Andrew Pinski



> Thanks,
> Tamar
> >
> > OK.
> >
> > > gcc/ChangeLog:
> > >
> > >         PR rtl-optimization/112483
> > >         * simplify-rtx.cc (simplify_binary_operation_1) <case
> COPYSIGN>:
> > >         Fix the simplification of (fcopysign x, NEGATIVE_CONST).
> > > ---
> > >
> > > Bootstrapped and regtested on loongarch64-linux-gnu and
> > > x86_64-linux-gnu.  Ok for trunk?
> > >
> > >  gcc/simplify-rtx.cc | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/gcc/simplify-rtx.cc b/gcc/simplify-rtx.cc index
> > > 69d87579d9c..2d2e5a3c1ca 100644
> > > --- a/gcc/simplify-rtx.cc
> > > +++ b/gcc/simplify-rtx.cc
> > > @@ -4392,7 +4392,7 @@ simplify_ashift:
> > >           real_convert (&f1, mode, CONST_DOUBLE_REAL_VALUE (trueop1));
> > >           rtx tmp = simplify_gen_unary (ABS, mode, op0, mode);
> > >           if (REAL_VALUE_NEGATIVE (f1))
> > > -           tmp = simplify_gen_unary (NEG, mode, op0, mode);
> > > +           tmp = simplify_gen_unary (NEG, mode, tmp, mode);
> > >           return tmp;
> > >         }
> > >        if (GET_CODE (op0) == NEG || GET_CODE (op0) == ABS)
> > > --
> > > 2.42.1
> > >
>