*[PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)@ 2017-06-24 21:56 Andrew Pinski2017-06-25 8:28 ` Marc Glisse 0 siblings, 1 reply; 16+ messages in thread From: Andrew Pinski @ 2017-06-24 21:56 UTC (permalink / raw) To: GCC Patches [-- Attachment #1: Type: text/plain, Size: 1520 bytes --] 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. [-- Attachment #2: copysign-1.diff.txt --] [-- Type: text/plain, Size: 4852 bytes --] Index: match.pd =================================================================== --- match.pd (revision 249626) +++ match.pd (working copy) @@ -155,6 +155,57 @@ || !COMPLEX_FLOAT_TYPE_P (type))) (negate @0))) +(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)))))) + /* 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))) + +/* 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)) + /* X * 1, X / 1 -> X. */ (for op (mult trunc_div ceil_div floor_div round_div exact_div) (simplify Index: testsuite/gcc.dg/tree-ssa/copy-sign-1.c =================================================================== --- testsuite/gcc.dg/tree-ssa/copy-sign-1.c (nonexistent) +++ testsuite/gcc.dg/tree-ssa/copy-sign-1.c (working copy) @@ -0,0 +1,35 @@ +/* { dg-options "-O2 -ffast-math -fdump-tree-gimple" } */ +/* { dg-do compile } */ +float f(float x) +{ + return (x > 0.f ? -1.f : 1.f); +} +float f1(float x) +{ + return (x > 0.f ? 1.f : -1.f); +} +float g(float x) +{ + return (x >= 0.f ? -1.f : 1.f); +} +float g1(float x) +{ + return (x >= 0.f ? 1.f : -1.f); +} +float h(float x) +{ + return (x < 0.f ? -1.f : 1.f); +} +float h1(float x) +{ + return (x < 0.f ? 1.f : -1.f); +} +float i(float x) +{ + return (x <= 0.f ? -1.f : 1.f); +} +float i1(float x) +{ + return (x <= 0.f ? 1.f : -1.f); +} +/* { dg-final { scan-tree-dump-times "copysign" 8 "gimple"} } */ Index: testsuite/gcc.dg/tree-ssa/copy-sign-2.c =================================================================== --- testsuite/gcc.dg/tree-ssa/copy-sign-2.c (nonexistent) +++ testsuite/gcc.dg/tree-ssa/copy-sign-2.c (working copy) @@ -0,0 +1,13 @@ +/* { dg-options "-O2 -ffast-math -fdump-tree-optimized" } */ +/* { dg-do compile } */ +float f(float x) +{ + float t = __builtin_copysignf (1.0f, x); + return x * t; +} +float f1(float x) +{ + float t = __builtin_copysignf (1.0f, -x); + return x * t; +} +/* { dg-final { scan-tree-dump-times "ABS" 2 "optimized"} } */ Index: testsuite/gcc.dg/tree-ssa/mult-abs-2.c =================================================================== --- testsuite/gcc.dg/tree-ssa/mult-abs-2.c (nonexistent) +++ testsuite/gcc.dg/tree-ssa/mult-abs-2.c (working copy) @@ -0,0 +1,35 @@ +/* { dg-options "-O2 -ffast-math -fdump-tree-gimple" } */ +/* { dg-do compile } */ +float f(float x) +{ + return x * (x > 0.f ? -1.f : 1.f); +} +float f1(float x) +{ + return x * (x > 0.f ? 1.f : -1.f); +} +float g(float x) +{ + return x * (x >= 0.f ? -1.f : 1.f); +} +float g1(float x) +{ + return x * (x >= 0.f ? 1.f : -1.f); +} +float h(float x) +{ + return x * (x < 0.f ? -1.f : 1.f); +} +float h1(float x) +{ + return x * (x < 0.f ? 1.f : -1.f); +} +float i(float x) +{ + return x * (x <= 0.f ? -1.f : 1.f); +} +float i1(float x) +{ + return x * (x <= 0.f ? 1.f : -1.f); +} +/* { dg-final { scan-tree-dump-times "ABS" 8 "gimple"} } */ ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-24 21:56 [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a) Andrew Pinski@ 2017-06-25 8:28 ` Marc Glisse2017-06-25 18:18 ` Andrew Pinski 2017-06-26 8:45 ` Richard Sandiford 0 siblings, 2 replies; 16+ messages in thread From: Marc Glisse @ 2017-06-25 8:28 UTC (permalink / raw) To: Andrew Pinski;+Cc:GCC Patches +(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? -- Marc Glisse ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-25 8:28 ` Marc Glisse@ 2017-06-25 18:18 ` Andrew Pinski2017-06-25 21:28 ` Andrew Pinski 2017-06-26 8:45 ` Richard Sandiford 1 sibling, 1 reply; 16+ messages in thread From: Andrew Pinski @ 2017-06-25 18:18 UTC (permalink / raw) To: GCC Patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-25 18:18 ` Andrew Pinski@ 2017-06-25 21:28 ` Andrew Pinski2017-06-27 8:49 ` Richard Biener ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: Andrew Pinski @ 2017-06-25 21:28 UTC (permalink / raw) To: GCC Patches [-- Attachment #1: Type: text/plain, Size: 4675 bytes --] On Sun, Jun 25, 2017 at 11:18 AM, Andrew Pinski <pinskia@gmail.com> wrote: > 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. New patch. 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, CST)): 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. > > Thanks, > Andrew > >> >> -- >> Marc Glisse [-- Attachment #2: copysign-1.diff.txt --] [-- Type: text/plain, Size: 4754 bytes --] Index: match.pd =================================================================== --- match.pd (revision 249626) +++ match.pd (working copy) @@ -155,6 +155,58 @@ || !COMPLEX_FLOAT_TYPE_P (type))) (negate @0))) +(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@1 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 @1 (outp @0))) + (if (types_match (type, double_type_node)) + (BUILT_IN_COPYSIGN @1 (outp @0))) + (if (types_match (type, long_double_type_node)) + (BUILT_IN_COPYSIGNL @1 (outp @0)))))) + /* 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@1) + (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 @1 (outn @0))) + (if (types_match (type, double_type_node)) + (BUILT_IN_COPYSIGN @1 (outn @0))) + (if (types_match (type, long_double_type_node)) + (BUILT_IN_COPYSIGNL @1 (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))) + +/* 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 (CST, X) into copysign (ABS(CST), X). */ +(simplify + (COPYSIGN REAL_CST@0 @1) + (if (REAL_VALUE_NEGATIVE (TREE_REAL_CST (@0))) + (COPYSIGN (negate @0) @1))) + /* X * 1, X / 1 -> X. */ (for op (mult trunc_div ceil_div floor_div round_div exact_div) (simplify Index: testsuite/gcc.dg/tree-ssa/copy-sign-1.c =================================================================== --- testsuite/gcc.dg/tree-ssa/copy-sign-1.c (nonexistent) +++ testsuite/gcc.dg/tree-ssa/copy-sign-1.c (working copy) @@ -0,0 +1,35 @@ +/* { dg-options "-O2 -ffast-math -fdump-tree-gimple" } */ +/* { dg-do compile } */ +float f(float x) +{ + return (x > 0.f ? -1.f : 1.f); +} +float f1(float x) +{ + return (x > 0.f ? 1.f : -1.f); +} +float g(float x) +{ + return (x >= 0.f ? -1.f : 1.f); +} +float g1(float x) +{ + return (x >= 0.f ? 1.f : -1.f); +} +float h(float x) +{ + return (x < 0.f ? -1.f : 1.f); +} +float h1(float x) +{ + return (x < 0.f ? 1.f : -1.f); +} +float i(float x) +{ + return (x <= 0.f ? -1.f : 1.f); +} +float i1(float x) +{ + return (x <= 0.f ? 1.f : -1.f); +} +/* { dg-final { scan-tree-dump-times "copysign" 8 "gimple"} } */ Index: testsuite/gcc.dg/tree-ssa/copy-sign-2.c =================================================================== --- testsuite/gcc.dg/tree-ssa/copy-sign-2.c (nonexistent) +++ testsuite/gcc.dg/tree-ssa/copy-sign-2.c (working copy) @@ -0,0 +1,13 @@ +/* { dg-options "-O2 -ffast-math -fdump-tree-optimized" } */ +/* { dg-do compile } */ +float f(float x) +{ + float t = __builtin_copysignf (1.0f, x); + return x * t; +} +float f1(float x) +{ + float t = __builtin_copysignf (1.0f, -x); + return x * t; +} +/* { dg-final { scan-tree-dump-times "ABS" 2 "optimized"} } */ Index: testsuite/gcc.dg/tree-ssa/mult-abs-2.c =================================================================== --- testsuite/gcc.dg/tree-ssa/mult-abs-2.c (nonexistent) +++ testsuite/gcc.dg/tree-ssa/mult-abs-2.c (working copy) @@ -0,0 +1,35 @@ +/* { dg-options "-O2 -ffast-math -fdump-tree-gimple" } */ +/* { dg-do compile } */ +float f(float x) +{ + return x * (x > 0.f ? -1.f : 1.f); +} +float f1(float x) +{ + return x * (x > 0.f ? 1.f : -1.f); +} +float g(float x) +{ + return x * (x >= 0.f ? -1.f : 1.f); +} +float g1(float x) +{ + return x * (x >= 0.f ? 1.f : -1.f); +} +float h(float x) +{ + return x * (x < 0.f ? -1.f : 1.f); +} +float h1(float x) +{ + return x * (x < 0.f ? 1.f : -1.f); +} +float i(float x) +{ + return x * (x <= 0.f ? -1.f : 1.f); +} +float i1(float x) +{ + return x * (x <= 0.f ? 1.f : -1.f); +} +/* { dg-final { scan-tree-dump-times "ABS" 8 "gimple"} } */ ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-25 8:28 ` Marc Glisse 2017-06-25 18:18 ` Andrew Pinski@ 2017-06-26 8:45 ` Richard Sandiford2017-06-26 15:22 ` Joseph Myers 1 sibling, 1 reply; 16+ messages in thread From: Richard Sandiford @ 2017-06-26 8:45 UTC (permalink / raw) To: Marc Glisse;+Cc:Andrew Pinski, GCC Patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-26 8:45 ` Richard Sandiford@ 2017-06-26 15:22 ` Joseph Myers2017-06-26 18:23 ` Richard Sandiford 0 siblings, 1 reply; 16+ messages in thread From: Joseph Myers @ 2017-06-26 15:22 UTC (permalink / raw) To: Richard Sandiford;+Cc:Marc Glisse, Andrew Pinski, GCC Patches 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 S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 16+ messages in thread

*2017-06-26 15:22 ` Joseph MyersRe: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)@ 2017-06-26 18:23 ` Richard Sandiford0 siblings, 0 replies; 16+ messages in thread From: Richard Sandiford @ 2017-06-26 18:23 UTC (permalink / raw) To: Joseph Myers;+Cc:Marc Glisse, Andrew Pinski, GCC Patches 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 ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-25 21:28 ` Andrew Pinski@ 2017-06-27 8:49 ` Richard Biener2017-06-27 14:52 ` Tamar Christina ` (2 subsequent siblings) 3 siblings, 0 replies; 16+ messages in thread From: Richard Biener @ 2017-06-27 8:49 UTC (permalink / raw) To: Andrew Pinski;+Cc:GCC Patches On Sun, Jun 25, 2017 at 11:28 PM, Andrew Pinski <pinskia@gmail.com> wrote: > On Sun, Jun 25, 2017 at 11:18 AM, Andrew Pinski <pinskia@gmail.com> wrote: >> 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. > > New patch. > OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions. Ok. We can see if the discussion around using internal functions can end up improving things here later. We've several cases in match.pd dealing with generating of {,f,l} variants. Richard. > 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, CST)): 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. > >> >> Thanks, >> Andrew >> >>> >>> -- >>> Marc Glisse ^ permalink raw reply [flat|nested] 16+ messages in thread

*RE: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-25 21:28 ` Andrew Pinski 2017-06-27 8:49 ` Richard Biener@ 2017-06-27 14:52 ` Tamar Christina2017-06-27 14:57 ` Richard Biener 2017-06-28 8:50 ` Christophe Lyon 2017-06-29 18:49 ` H.J. Lu 3 siblings, 1 reply; 16+ messages in thread From: Tamar Christina @ 2017-06-27 14:52 UTC (permalink / raw) To: Andrew Pinski, GCC Patches;+Cc:nd > >> +(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)))))) > >> Hi, Out of curiosity is there any reason why this transformation can't be more general? e.g. Transform (X > 0.0 ? CST : -CST) into copysign(CST, X). we would at the very least avoid a csel or a branch then. Regards, Tamar ^ permalink raw reply [flat|nested] 16+ messages in thread

*RE: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-27 14:52 ` Tamar Christina@ 2017-06-27 14:57 ` Richard Biener2017-06-27 17:52 ` Andrew Pinski 0 siblings, 1 reply; 16+ messages in thread From: Richard Biener @ 2017-06-27 14:57 UTC (permalink / raw) To: gcc-patches, Tamar Christina, Andrew Pinski, GCC Patches;+Cc:nd On June 27, 2017 4:52:28 PM GMT+02:00, Tamar Christina <Tamar.Christina@arm.com> 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)))))) >> >> > >Hi, > >Out of curiosity is there any reason why this transformation can't be >more general? > >e.g. Transform (X > 0.0 ? CST : -CST) into copysign(CST, X). That's also possible, yes. >we would at the very least avoid a csel or a branch then. > >Regards, >Tamar ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-27 14:57 ` Richard Biener@ 2017-06-27 17:52 ` Andrew Pinski2017-06-28 7:37 ` Jakub Jelinek 0 siblings, 1 reply; 16+ messages in thread From: Andrew Pinski @ 2017-06-27 17:52 UTC (permalink / raw) To: Richard Biener;+Cc:Tamar Christina, GCC Patches, nd On Tue, Jun 27, 2017 at 7:56 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On June 27, 2017 4:52:28 PM GMT+02:00, Tamar Christina <Tamar.Christina@arm.com> 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)))))) >>> >> >> >>Hi, >> >>Out of curiosity is there any reason why this transformation can't be >>more general? >> >>e.g. Transform (X > 0.0 ? CST : -CST) into copysign(CST, X). > > That's also possible, yes. I will be implementing that latter today. Thanks, Andrew Pinski > >>we would at the very least avoid a csel or a branch then. >> >>Regards, >>Tamar > ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-27 17:52 ` Andrew Pinski@ 2017-06-28 7:37 ` Jakub Jelinek2017-06-28 7:57 ` Uros Bizjak 0 siblings, 1 reply; 16+ messages in thread From: Jakub Jelinek @ 2017-06-28 7:37 UTC (permalink / raw) To: Andrew Pinski, Uros Bizjak Cc: Richard Biener, Tamar Christina, GCC Patches, nd On Tue, Jun 27, 2017 at 10:52:47AM -0700, Andrew Pinski wrote: > On Tue, Jun 27, 2017 at 7:56 AM, Richard Biener > <richard.guenther@gmail.com> wrote: > > On June 27, 2017 4:52:28 PM GMT+02:00, Tamar Christina <Tamar.Christina@arm.com> 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)))))) The patch regressed the gcc.target/i386/cmov7.c testcase. From the description in the testcase it was testing the combiner, so I've transformed it into something that also tests the combiner the same way, ok for trunk? That said, for copysign if we match it we don't emit a fcmov while it might be a good idea. 2017-06-28 Jakub Jelinek <jakub@redhat.com> * gcc.target/i386/cmov7.c (sgn): Renamed to ... (foo): ... this. Change constants such that it isn't matched as __builtin_copysign, yet tests the combiner the same. --- gcc/testsuite/gcc.target/i386/cmov7.c.jj 2016-05-22 12:20:23.000000000 +0200 +++ gcc/testsuite/gcc.target/i386/cmov7.c 2017-06-28 09:20:24.000000000 +0200 @@ -10,7 +10,7 @@ (set (reg:DF) (float_extend:DF (mem:SF (symbol_ref...)))). */ double -sgn (double __x) +foo (double __x) { - return __x >= 0.0 ? 1.0 : -1.0; + return __x >= 1.0 ? 0.0 : -1.0; } Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-28 7:37 ` Jakub Jelinek@ 2017-06-28 7:57 ` Uros Bizjak0 siblings, 0 replies; 16+ messages in thread From: Uros Bizjak @ 2017-06-28 7:57 UTC (permalink / raw) To: Jakub Jelinek Cc: Andrew Pinski, Richard Biener, Tamar Christina, GCC Patches, nd On Wed, Jun 28, 2017 at 9:37 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Jun 27, 2017 at 10:52:47AM -0700, Andrew Pinski wrote: >> On Tue, Jun 27, 2017 at 7:56 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >> > On June 27, 2017 4:52:28 PM GMT+02:00, Tamar Christina <Tamar.Christina@arm.com> 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)))))) > > The patch regressed the gcc.target/i386/cmov7.c testcase. > From the description in the testcase it was testing the combiner, so I've > transformed it into something that also tests the combiner the same way, > ok for trunk? That said, for copysign if we match it we don't emit a fcmov > while it might be a good idea. > > 2017-06-28 Jakub Jelinek <jakub@redhat.com> > > * gcc.target/i386/cmov7.c (sgn): Renamed to ... > (foo): ... this. Change constants such that it isn't matched > as __builtin_copysign, yet tests the combiner the same. OK. Thanks, Uros. > --- gcc/testsuite/gcc.target/i386/cmov7.c.jj 2016-05-22 12:20:23.000000000 +0200 > +++ gcc/testsuite/gcc.target/i386/cmov7.c 2017-06-28 09:20:24.000000000 +0200 > @@ -10,7 +10,7 @@ > (set (reg:DF) (float_extend:DF (mem:SF (symbol_ref...)))). */ > > double > -sgn (double __x) > +foo (double __x) > { > - return __x >= 0.0 ? 1.0 : -1.0; > + return __x >= 1.0 ? 0.0 : -1.0; > } > > > Jakub ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-25 21:28 ` Andrew Pinski 2017-06-27 8:49 ` Richard Biener 2017-06-27 14:52 ` Tamar Christina@ 2017-06-28 8:50 ` Christophe Lyon2017-06-28 9:29 ` Richard Biener 2017-06-29 18:49 ` H.J. Lu 3 siblings, 1 reply; 16+ messages in thread From: Christophe Lyon @ 2017-06-28 8:50 UTC (permalink / raw) To: Andrew Pinski;+Cc:GCC Patches [-- Attachment #1: Type: text/plain, Size: 5180 bytes --] On 25 June 2017 at 23:28, Andrew Pinski <pinskia@gmail.com> wrote: > On Sun, Jun 25, 2017 at 11:18 AM, Andrew Pinski <pinskia@gmail.com> wrote: >> 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. > > New patch. > OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions. > Hi Andrew, 2 of the new testcases fail on aarch64*-elf: FAIL: gcc.dg/tree-ssa/copy-sign-1.c scan-tree-dump-times gimple "copysign" 8 FAIL: gcc.dg/tree-ssa/mult-abs-2.c scan-tree-dump-times gimple "ABS" 8 The attached patch makes them unsupported by requiring c99_runtime effective-target. OK? > 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, CST)): 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. > >> >> Thanks, >> Andrew >> >>> >>> -- >>> Marc Glisse [-- Attachment #2: match-testcases.chlog.txt --] [-- Type: text/plain, Size: 200 bytes --] 2017-06-28 Christophe Lyon <christophe.lyon@linaro.org> gcc/testsuite/ * gcc.dg/tree-ssa/copy-sign-1.c: Add c99_runtime effective target and options. * gcc.dg/tree-ssa/mult-abs-2.c: Likewise. [-- Attachment #3: match-testcases.patch.txt --] [-- Type: text/plain, Size: 1074 bytes --] diff --git a/gcc/testsuite/gcc.dg/tree-ssa/copy-sign-1.c b/gcc/testsuite/gcc.dg/tree-ssa/copy-sign-1.c index 9ebdf50..de3e7b2 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/copy-sign-1.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/copy-sign-1.c @@ -1,5 +1,7 @@ -/* { dg-options "-O2 -ffast-math -fdump-tree-gimple" } */ /* { dg-do compile } */ +/* { dg-require-effective-target c99_runtime } */ +/* { dg-options "-O2 -ffast-math -fdump-tree-gimple" } */ +/* { dg-add-options c99_runtime } */ float f(float x) { return (x > 0.f ? -1.f : 1.f); diff --git a/gcc/testsuite/gcc.dg/tree-ssa/mult-abs-2.c b/gcc/testsuite/gcc.dg/tree-ssa/mult-abs-2.c index b6a1a79..d74ba2f 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/mult-abs-2.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/mult-abs-2.c @@ -1,5 +1,8 @@ -/* { dg-options "-O2 -ffast-math -fdump-tree-gimple" } */ /* { dg-do compile } */ +/* { dg-require-effective-target c99_runtime } */ +/* { dg-options "-O2 -ffast-math -fdump-tree-gimple" } */ +/* { dg-add-options c99_runtime } */ + float f(float x) { return x * (x > 0.f ? -1.f : 1.f); ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-28 8:50 ` Christophe Lyon@ 2017-06-28 9:29 ` Richard Biener0 siblings, 0 replies; 16+ messages in thread From: Richard Biener @ 2017-06-28 9:29 UTC (permalink / raw) To: Christophe Lyon;+Cc:Andrew Pinski, GCC Patches On Wed, Jun 28, 2017 at 10:50 AM, Christophe Lyon <christophe.lyon@linaro.org> wrote: > On 25 June 2017 at 23:28, Andrew Pinski <pinskia@gmail.com> wrote: >> On Sun, Jun 25, 2017 at 11:18 AM, Andrew Pinski <pinskia@gmail.com> wrote: >>> 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. >> >> New patch. >> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions. >> > Hi Andrew, > > 2 of the new testcases fail on aarch64*-elf: > FAIL: gcc.dg/tree-ssa/copy-sign-1.c scan-tree-dump-times gimple "copysign" 8 > FAIL: gcc.dg/tree-ssa/mult-abs-2.c scan-tree-dump-times gimple "ABS" 8 > > The attached patch makes them unsupported by requiring c99_runtime > effective-target. > > OK? Ok. > >> 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, CST)): 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. >> >>> >>> Thanks, >>> Andrew >>> >>>> >>>> -- >>>> Marc Glisse ^ permalink raw reply [flat|nested] 16+ messages in thread

*Re: [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a)2017-06-25 21:28 ` Andrew Pinski ` (2 preceding siblings ...) 2017-06-28 8:50 ` Christophe Lyon@ 2017-06-29 18:49 ` H.J. Lu3 siblings, 0 replies; 16+ messages in thread From: H.J. Lu @ 2017-06-29 18:49 UTC (permalink / raw) To: Andrew Pinski;+Cc:GCC Patches On Sun, Jun 25, 2017 at 2:28 PM, Andrew Pinski <pinskia@gmail.com> wrote: > On Sun, Jun 25, 2017 at 11:18 AM, Andrew Pinski <pinskia@gmail.com> wrote: >> 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. > > New patch. > 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, CST)): 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. > This caused: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81255 -- H.J. ^ permalink raw reply [flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-06-29 18:49 UTC | newest]Thread overview:16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-24 21:56 [PATCH] Fold (a > 0 ? 1.0 : -1.0) into copysign (1.0, a) and a * copysign (1.0, a) into abs(a) Andrew Pinski 2017-06-25 8:28 ` Marc Glisse 2017-06-25 18:18 ` Andrew Pinski 2017-06-25 21:28 ` Andrew Pinski 2017-06-27 8:49 ` Richard Biener 2017-06-27 14:52 ` Tamar Christina 2017-06-27 14:57 ` Richard Biener 2017-06-27 17:52 ` Andrew Pinski 2017-06-28 7:37 ` Jakub Jelinek 2017-06-28 7:57 ` Uros Bizjak 2017-06-28 8:50 ` Christophe Lyon 2017-06-28 9:29 ` Richard Biener 2017-06-29 18:49 ` H.J. Lu 2017-06-26 8:45 ` Richard Sandiford 2017-06-26 15:22 ` Joseph Myers 2017-06-26 18:23 ` Richard Sandiford

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).