Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)

Hi,
  I decided to split my previous patch into two different patches, one
for floating point and one for integer.  This is the floating point
version as we can use copysign here which should allow for more cases
to be optimized and even by a phiopt which uses match-and-simplify (I
hope to be able to submit that during stage 1).

This patch adds the following transformations:
Transform (X > 0.0 ? 1.0 : -1.0) into copysign(1, X).
Transform (X >= 0.0 ? 1.0 : -1.0) into copysign(1, X).
Transform (X < 0.0 ? 1.0 : -1.0) into copysign(1,-X).
Transform (X <= 0.0 ? 1.0 : -1.0) into copysign(1,-X).
Transform (X > 0.0 ? -1.0 : 1.0) into copysign(1,-X).
Transform (X >= 0.0 ? -1.0 : 1.0) into copysign(1,-X).
Transform (X < 0.0 ? -1.0 : 1.0) into copysign(1,X).
Transform (X <= 0.0 ? -1.0 : 1.0) into copysign(1,X).

Transform X * copysign (1.0, X) into abs(X).
Transform X * copysign (1.0, -X) into -abs(X).

Transform copysign (-1.0, X) into copysign (1.0, X).

The last one is there so if someone decides to writes -1.0 instead of
1.0 in the code we would get the optimization still.

OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.

Thanks,
Andrew Pinski

ChangeLog:
* match.pd (X >/>=/</<= 0 ? 1.0 : -1.0): New patterns.
(X * copysign (1.0, X)): New pattern.
(X * copysign (1.0, -X)): New pattern.
(copysign (-1.0, X)): New pattern.

testsuite/ChangeLog:
* gcc.dg/tree-ssa/copy-sign-1.c: New testcase.
* gcc.dg/tree-ssa/copy-sign-2.c: New testcase.
* gcc.dg/tree-ssa/mult-abs-2.c: New testcase.

+(for cmp (gt ge lt le)
+     outp (convert convert negate negate)
+     outn (negate negate convert convert)
+ /* Transform (X > 0.0 ? 1.0 : -1.0) into copysign(1, X). */
+ /* Transform (X >= 0.0 ? 1.0 : -1.0) into copysign(1, X). */
+ /* Transform (X < 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
+ /* Transform (X <= 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
+ (simplify
+  (cond (cmp @0 real_zerop) real_onep real_minus_onep)
+  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type)
+       && types_match (type, TREE_TYPE (@0)))
+   (switch
+    (if (types_match (type, float_type_node))
+     (BUILT_IN_COPYSIGNF { build_one_cst (type); } (outp @0)))
+    (if (types_match (type, double_type_node))
+     (BUILT_IN_COPYSIGN { build_one_cst (type); } (outp @0)))
+    (if (types_match (type, long_double_type_node))
+     (BUILT_IN_COPYSIGNL { build_one_cst (type); } (outp @0))))))

There is already a 1.0 of the right type in the input, it would be easier 
to reuse it in the output than build a new one.

Non-generic builtins like copysign are such a pain... We also end up 
missing the 128-bit case that way (pre-existing problem, not your patch). 
We seem to have a corresponding internal function, but apparently it is 
not used until expansion (well, maybe during vectorization).

+ /* Transform (X > 0.0 ? -1.0 : 1.0) into copysign(1,-X). */
+ /* Transform (X >= 0.0 ? -1.0 : 1.0) into copysign(1,-X). */
+ /* Transform (X < 0.0 ? -1.0 : 1.0) into copysign(1,X). */
+ /* Transform (X <= 0.0 ? -1.0 : 1.0) into copysign(1,X). */
+ (simplify
+  (cond (cmp @0 real_zerop) real_minus_onep real_onep)
+  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type)
+       && types_match (type, TREE_TYPE (@0)))
+   (switch
+    (if (types_match (type, float_type_node))
+     (BUILT_IN_COPYSIGNF { build_one_cst (type); } (outn @0)))
+    (if (types_match (type, double_type_node))
+     (BUILT_IN_COPYSIGN { build_one_cst (type); } (outn @0)))
+    (if (types_match (type, long_double_type_node))
+     (BUILT_IN_COPYSIGNL { build_one_cst (type); } (outn @0)))))))
+
+/* Transform X * copysign (1.0, X) into abs(X). */
+(simplify
+ (mult:c @0 (COPYSIGN real_onep @0))
+ (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type))
+  (abs @0)))

I would have expected it do to the right thing for signed zero and qNaN. 
Can you describe a case where it would give the wrong result, or are the 
conditions just conservative?

+/* Transform X * copysign (1.0, -X) into -abs(X). */
+(simplify
+ (mult:c @0 (COPYSIGN real_onep (negate @0)))
+ (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type))
+  (negate (abs @0))))
+
+/* Transform copysign (-1.0, X) into copysign (1.0, X). */
+(simplify
+ (COPYSIGN real_minus_onep @0)
+ (COPYSIGN { build_one_cst (type); } @0))

(simplify
  (COPYSIGN REAL_CST@0 @1)
  (if (REAL_VALUE_NEGATIVE (TREE_REAL_CST (@0)))
   (COPYSIGN (negate @0) @1)))
? Or does that create trouble with sNaN and only the 1.0 case is worth
the trouble?

On Sun, Jun 25, 2017 at 1:28 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> +(for cmp (gt ge lt le)
> +     outp (convert convert negate negate)
> +     outn (negate negate convert convert)
> + /* Transform (X > 0.0 ? 1.0 : -1.0) into copysign(1, X). */
> + /* Transform (X >= 0.0 ? 1.0 : -1.0) into copysign(1, X). */
> + /* Transform (X < 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
> + /* Transform (X <= 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
> + (simplify
> +  (cond (cmp @0 real_zerop) real_onep real_minus_onep)
> +  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type)
> +       && types_match (type, TREE_TYPE (@0)))
> +   (switch
> +    (if (types_match (type, float_type_node))
> +     (BUILT_IN_COPYSIGNF { build_one_cst (type); } (outp @0)))
> +    (if (types_match (type, double_type_node))
> +     (BUILT_IN_COPYSIGN { build_one_cst (type); } (outp @0)))
> +    (if (types_match (type, long_double_type_node))
> +     (BUILT_IN_COPYSIGNL { build_one_cst (type); } (outp @0))))))
>
> There is already a 1.0 of the right type in the input, it would be easier to
> reuse it in the output than build a new one.

Right.  Fixed.

>
> Non-generic builtins like copysign are such a pain... We also end up missing
> the 128-bit case that way (pre-existing problem, not your patch). We seem to
> have a corresponding internal function, but apparently it is not used until
> expansion (well, maybe during vectorization).

Yes I noticed that while working on a different patch related to
copysign; The generic version of a*copysign(1.0, b) [see the other
thread where the ARM folks started a patch for it; yes it was by pure
accident that I was working on this and really did not notice that
thread until yesterday].
I was looking into a nice way of creating copysign without having to
do the switch but I could not find one.  In the end I copied was done
already in a different location in match.pd; this is also the reason
why I had the build_one_cst there.

>
> + /* Transform (X > 0.0 ? -1.0 : 1.0) into copysign(1,-X). */
> + /* Transform (X >= 0.0 ? -1.0 : 1.0) into copysign(1,-X). */
> + /* Transform (X < 0.0 ? -1.0 : 1.0) into copysign(1,X). */
> + /* Transform (X <= 0.0 ? -1.0 : 1.0) into copysign(1,X). */
> + (simplify
> +  (cond (cmp @0 real_zerop) real_minus_onep real_onep)
> +  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type)
> +       && types_match (type, TREE_TYPE (@0)))
> +   (switch
> +    (if (types_match (type, float_type_node))
> +     (BUILT_IN_COPYSIGNF { build_one_cst (type); } (outn @0)))
> +    (if (types_match (type, double_type_node))
> +     (BUILT_IN_COPYSIGN { build_one_cst (type); } (outn @0)))
> +    (if (types_match (type, long_double_type_node))
> +     (BUILT_IN_COPYSIGNL { build_one_cst (type); } (outn @0)))))))
> +
> +/* Transform X * copysign (1.0, X) into abs(X). */
> +(simplify
> + (mult:c @0 (COPYSIGN real_onep @0))
> + (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type))
> +  (abs @0)))
>
> I would have expected it do to the right thing for signed zero and qNaN. Can
> you describe a case where it would give the wrong result, or are the
> conditions just conservative?

I was just being conservative; maybe too conservative but I was a bit
worried I could get it incorrect.

>
> +/* Transform X * copysign (1.0, -X) into -abs(X). */
> +(simplify
> + (mult:c @0 (COPYSIGN real_onep (negate @0)))
> + (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type))
> +  (negate (abs @0))))
> +
> +/* Transform copysign (-1.0, X) into copysign (1.0, X). */
> +(simplify
> + (COPYSIGN real_minus_onep @0)
> + (COPYSIGN { build_one_cst (type); } @0))
>
> (simplify
>  (COPYSIGN REAL_CST@0 @1)
>  (if (REAL_VALUE_NEGATIVE (TREE_REAL_CST (@0)))
>   (COPYSIGN (negate @0) @1)))
> ? Or does that create trouble with sNaN and only the 1.0 case is worth
> the trouble?

No that is the correct way; I Noticed the other thread about copysign
had something similar as what should be done too.

I will send out a new patch after testing soon.

Thanks,
Andrew

>
> --
> Marc Glisse

Marc Glisse <marc.glisse@inria.fr> writes:
> +(for cmp (gt ge lt le)
> +     outp (convert convert negate negate)
> +     outn (negate negate convert convert)
> + /* Transform (X > 0.0 ? 1.0 : -1.0) into copysign(1, X). */
> + /* Transform (X >= 0.0 ? 1.0 : -1.0) into copysign(1, X). */
> + /* Transform (X < 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
> + /* Transform (X <= 0.0 ? 1.0 : -1.0) into copysign(1,-X). */
> + (simplify
> +  (cond (cmp @0 real_zerop) real_onep real_minus_onep)
> +  (if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type)
> +       && types_match (type, TREE_TYPE (@0)))
> +   (switch
> +    (if (types_match (type, float_type_node))
> +     (BUILT_IN_COPYSIGNF { build_one_cst (type); } (outp @0)))
> +    (if (types_match (type, double_type_node))
> +     (BUILT_IN_COPYSIGN { build_one_cst (type); } (outp @0)))
> +    (if (types_match (type, long_double_type_node))
> +     (BUILT_IN_COPYSIGNL { build_one_cst (type); } (outp @0))))))
>
> There is already a 1.0 of the right type in the input, it would be easier 
> to reuse it in the output than build a new one.
>
> Non-generic builtins like copysign are such a pain... We also end up 
> missing the 128-bit case that way (pre-existing problem, not your patch). 
> We seem to have a corresponding internal function, but apparently it is 
> not used until expansion (well, maybe during vectorization).

It should be OK to introduce uses of the internal functions whenever
it's useful.  The match code will check that the internal function is
implemented before allowing the transformation.

The idea was more-or-less:

- Leave calls to libm functions alone until expand if there's no
  particular benefit to converting them earlier.  This avoids introducing
  a gratuitous difference between targets that can and can't open-code the
  function.

- Fold calls to libm functions to calls to libm functions where
  possible, because these transformations work regardless of whether the
  function can be open-coded.

- When introducing new calls, use internal functions if we need to be
  sure that the target has an appropriate optab.

- Also use internal functions to represent the non-errno setting forms
  of an internal function, in cases where the built-in functions are
  assumed to set errno.

But IFN_COPYSIGN might not be useful as-is, since the copysign built-ins
are usually expanded without the help of an optab.  It should be possible
to change things so that IFN_COPYSIGN is supported in the same situations
as the built-in though.

Thanks,
Richard

On Mon, 26 Jun 2017, Richard Sandiford wrote:

> > Non-generic builtins like copysign are such a pain... We also end up 
> > missing the 128-bit case that way (pre-existing problem, not your patch). 
> > We seem to have a corresponding internal function, but apparently it is 
> > not used until expansion (well, maybe during vectorization).
> 
> It should be OK to introduce uses of the internal functions whenever
> it's useful.  The match code will check that the internal function is
> implemented before allowing the transformation.

How well would internal functions work with some having built-in functions 
only for float, double and long double, others (like copysign) having them 
for all the _FloatN and _FloatNx types?

(Preferably of course the built-in functions for libm functions would 
generally exist for all the types.  I didn't include that in my patches 
adding _FloatN/_FloatNx support 
<https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01290.html> 
<https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01442.html> and noted 
various issues to watch out for there, especially increasing the size of 
the enum of built-in functions and the startup cost of initializing them.  
There are other optimizations with similar issues of only covering float, 
double and long double.)

Joseph Myers <joseph@codesourcery.com> writes:
> On Mon, 26 Jun 2017, Richard Sandiford wrote:
>> > Non-generic builtins like copysign are such a pain... We also end up 
>> > missing the 128-bit case that way (pre-existing problem, not your patch). 
>> > We seem to have a corresponding internal function, but apparently it is 
>> > not used until expansion (well, maybe during vectorization).
>> 
>> It should be OK to introduce uses of the internal functions whenever
>> it's useful.  The match code will check that the internal function is
>> implemented before allowing the transformation.
>
> How well would internal functions work with some having built-in functions 
> only for float, double and long double, others (like copysign) having them 
> for all the _FloatN and _FloatNx types?

I don't think the internal functions themselves should be affected,
since they rely on optabs rather than external functions.  They can
support any mode that the target can support, even if there's no
equivalent standard function.

It might be a good idea to extend gencfn-macros.c and co. to handle
built-in functions that operate on _Float types though.  That wouldn't
be needed for correctness, but would be useful if we want to have folds
that operate on all built-in copysign functions as well as the internal
copysign function.

Thanks,
Richard