On 25 June 2017 at 23:28, Andrew Pinski <pins...@gmail.com> wrote:
> On Sun, Jun 25, 2017 at 11:18 AM, Andrew Pinski <pins...@gmail.com> wrote:
>> On Sun, Jun 25, 2017 at 1:28 AM, Marc Glisse <marc.gli...@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
2017-06-28 Christophe Lyon <christophe.l...@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.
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);
