Re: [PATCH] Simplify sinh (x) / cosh (x)
Hi Jeff, I've just checked and the proposed optimization is under -funsafe-math-optimizations and canonicalize_math_p(): `(if (flag_unsafe_math_optimizations && canonicalize_math_p ())` I'm sorry, but I don't quite understand what you mean with 'using those expanders in an unexpected way', should I put the optimization in another place and try to deal with the zero sign or is it fine the way it is? Rafael Tsuha Em ter, 10 de set de 2019 às 12:23, Jeff Law escreveu: > > On 9/10/19 1:36 AM, Uros Bizjak wrote: > > On Mon, Sep 9, 2019 at 8:44 PM Jeff Law wrote: > >> > >> On 9/4/19 12:16 PM, Rafael Tsuha wrote: > >>> Hi, Jeff > >>> > >>> Em seg, 29 de abr de 2019 às 18:22, Jeff Law escreveu: > >>>> > >>>> On 1/22/19 12:31 PM, Rafael Tsuha wrote: > >>>>> This patch simplifies the expression sinh (x) / cosh (x) to tanh (x). > >>>>> This rule is mathematically valid. > >>>>> > >>>>> There's a slight difference in the result when applying this > >>>>> optimization with x in the interval 0 < x <= 1e-4951. With the > >>>>> optimization, the result using long double is -0 and without the > >>>>> optimization, the result is +0. > >>>> That's an indication something has gone wrong. > >>>> > >>>> If I'm reading this correctly it sounds like tanh in that range is > >>>> returning -0? If so, that just seems like the wrong output from tanh > >>>> since IIUC for a positive input tanh will always have a positive output. > >>>> > >>> > >>> I reverted the patch sent to solve bug 88556 and found out that > >>> tanhl(0) started returning -0 after this patch. > >>> > >>> patch we reverted: > >>> (https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/i386/i386.c?r1=267325&r2=267324&pathrev=267325) > >>> > >>> In the line 44480 of this patch, it checks the sign bit of the input > >>> and if it's false the expression is multiplied by -1. In the way it's > >>> being calculated, this should be done only if the input is a number > >>> greater than zero. > >>> > >>> If we follow the code starting at line 44468, replacing op1 with 0, we > >>> can see that e2 equals 0 at line 44482, flags will be false and > >>> finally the e2 = -e2 operation will be executed generating the -0 > >>> result. > >>> > >>> I have implemented a testcase to reproduce the bug: > >>> https://paste.debian.net/1098800/ > >>> this code was compiled with -Ofast when we tested it. > >>> > >>> Should I file a bug about this? And for fixing, Is it a good solution > >>> to emit an instruction to return zero immediately if the input equals > >>> zero? > >> So if I'm understanding Uros's patch correctly, it's supposed to only be > >> used for -ffast-math where we don't necessarily honor signed zeros. > > > > True. The full patch is at [1], where it is evident that all these > > expanders are protected by flag_unsafe_math_optimizations. As > > explained in the patch sumbission, the equations are ported from [2], > > so barring some unwanted bug in the porting, they should be equal. I > > didn't analyse the correctness of the original equations. > It (your patch) looked fine to me given the -ffast-math constraint. > > I think the question we need to go back and answer is why the proposed > patch to improve sinh/cosh -> tanh is using those expanders in an > unexpected way. > > jeff
[PATCH] PR c/17896 Check for missplaced bitwise op
This patch adds a function to warn when there's a bitwise operation
between a boolean and any other type. This kind of operation is
probably a programmer mistake that may lead to unexpected behavior
because possibily the logical operation was intended.
The test was adapted from PR c/17896.
gcc/c-family/ChangeLog
2019-05-24 Rafael Tsuha
* c-warn.c (warn_logical_operator): Check for missplaced bitwise op.
gcc/testsuite/ChangeLog
2019-05-24 Rafael Tsuha
PR c/17896
* gcc.dg/boolean-bitwise.c: New test.
Index: gcc/c-family/c-warn.c
===
--- gcc/c-family/c-warn.c (revision 268782)
+++ gcc/c-family/c-warn.c (working copy)
@@ -167,10 +167,10 @@
}
/* Warn about uses of logical || / && operator in a context where it
- is likely that the bitwise equivalent was intended by the
- programmer. We have seen an expression in which CODE is a binary
- operator used to combine expressions OP_LEFT and OP_RIGHT, which before folding
- had CODE_LEFT and CODE_RIGHT, into an expression of type TYPE. */
+ is likely that the bitwise equivalent was intended by the programmer or vice
+ versa. We have seen an expression in which CODE is a binary operator used to
+ combine expressions OP_LEFT and OP_RIGHT, which before folding had CODE_LEFT
+ and CODE_RIGHT, into an expression of type TYPE. */
void
warn_logical_operator (location_t location, enum tree_code code, tree type,
@@ -178,6 +178,7 @@
enum tree_code ARG_UNUSED (code_right), tree op_right)
{
int or_op = (code == TRUTH_ORIF_EXPR || code == TRUTH_OR_EXPR);
+ int and_op = (code == TRUTH_ANDIF_EXPR || code == TRUTH_AND_EXPR);
int in0_p, in1_p, in_p;
tree low0, low1, low, high0, high1, high, lhs, rhs, tem;
bool strict_overflow_p = false;
@@ -188,7 +189,9 @@
if (code != TRUTH_ANDIF_EXPR
&& code != TRUTH_AND_EXPR
&& code != TRUTH_ORIF_EXPR
- && code != TRUTH_OR_EXPR)
+ && code != TRUTH_OR_EXPR
+ && code != BIT_AND_EXPR
+ && code != BIT_IOR_EXPR)
return;
/* We don't want to warn if either operand comes from a macro
@@ -219,7 +222,7 @@
if (or_op)
warning_at (location, OPT_Wlogical_op, "logical %"
" applied to non-boolean constant");
- else
+ else if (and_op)
warning_at (location, OPT_Wlogical_op, "logical %"
" applied to non-boolean constant");
TREE_NO_WARNING (op_left) = true;
@@ -227,6 +230,26 @@
}
}
+ /* Warn if &/| are being used in a context where it is
+ likely that the logical equivalent was intended by the
+ programmer. That is, an expression such as op_1 & op_2
+ where op_n should not be any boolean expression. */
+ if ( TREE_CODE (TREE_TYPE (op_right)) == BOOLEAN_TYPE
+ || TREE_CODE (TREE_TYPE (op_left)) == BOOLEAN_TYPE
+ || COMPARISON_CLASS_P ((op_left))
+ || COMPARISON_CLASS_P ((op_right)))
+{
+ tree folded_op_right = fold_for_warn (op_right);
+ if (code == BIT_IOR_EXPR)
+ warning_at (location, OPT_Wlogical_op, "bitwise %"
+ " applied to boolean type");
+ else if (code == BIT_AND_EXPR)
+ warning_at (location, OPT_Wlogical_op, "bitwise %"
+ " applied to boolean type");
+ TREE_NO_WARNING (op_left) = true;
+ return;
+ }
+
/* We do not warn for constants because they are typical of macro
expansions that test for features. */
if (CONSTANT_CLASS_P (fold_for_warn (op_left))
Index: gcc/testsuite/gcc.dg/boolean-bitwise.c
===
--- gcc/testsuite/gcc.dg/boolean-bitwise.c (nonexistent)
+++ gcc/testsuite/gcc.dg/boolean-bitwise.c (working copy)
@@ -0,0 +1,28 @@
+/* Test operation of -Wparentheses. booleans joined by & or | */
+
+/* { dg-do compile } */
+/* { dg-options "-Wlogical-op" } */
+
+int foo (int);
+
+int
+bar (int a, int b, int c)
+{
+ foo (a == b & b == c); /* { dg-warning "boolean" "correct warning" } */
+ foo (a == b & (b == c)); /* { dg-warning "boolean" "correct warning" } */
+ foo ((a == b) & b == c); /* { dg-warning "boolean" "correct warning" } */
+ foo (++a == b & b == c); /* { dg-warning "boolean" "correct warning" } */
+ foo ((a == b) & (b == c)); /* { dg-warning "boolean" "correct warning" } */
+ foo (a == b && b == c);
+ foo (a & b);
+
+ foo (a == b | b == c); /* { dg-warning "boolean" "correct warning" } */
+ foo (a == b | (b == c)); /* { dg-warning "boolean" "correct warning" } */
+ foo ((a == b) | b == c); /* { dg-warning "boolean" "correct warning" } */
+ foo (++a == b | b == c); /* { dg-warning "boolean" "correct warning" } */
+ foo ((a == b) | (b == c)); /* { dg-warning "boolean" "correct warning" } */
+ foo (a == b || b == c);
+ foo (a | b);
+
+ return 0;
+}
Re: [PATCH] PR c/17896 Check for missplaced bitwise op
ping Em sex, 24 de mai de 2019 às 09:53, Rafael Tsuha escreveu: > > This patch adds a function to warn when there's a bitwise operation > between a boolean and any other type. This kind of operation is > probably a programmer mistake that may lead to unexpected behavior > because possibily the logical operation was intended. > The test was adapted from PR c/17896. > > gcc/c-family/ChangeLog > 2019-05-24 Rafael Tsuha > > * c-warn.c (warn_logical_operator): Check for missplaced bitwise op. > > gcc/testsuite/ChangeLog > 2019-05-24 Rafael Tsuha > > PR c/17896 > * gcc.dg/boolean-bitwise.c: New test.
Re: [PATCH] Simplify sinh (x) / cosh (x)
Hi, Jeff Em seg, 29 de abr de 2019 às 18:22, Jeff Law escreveu: > > On 1/22/19 12:31 PM, Rafael Tsuha wrote: > > This patch simplifies the expression sinh (x) / cosh (x) to tanh (x). > > This rule is mathematically valid. > > > > There's a slight difference in the result when applying this > > optimization with x in the interval 0 < x <= 1e-4951. With the > > optimization, the result using long double is -0 and without the > > optimization, the result is +0. > That's an indication something has gone wrong. > > If I'm reading this correctly it sounds like tanh in that range is > returning -0? If so, that just seems like the wrong output from tanh > since IIUC for a positive input tanh will always have a positive output. > I reverted the patch sent to solve bug 88556 and found out that tanhl(0) started returning -0 after this patch. patch we reverted: (https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/i386/i386.c?r1=267325&r2=267324&pathrev=267325) In the line 44480 of this patch, it checks the sign bit of the input and if it's false the expression is multiplied by -1. In the way it's being calculated, this should be done only if the input is a number greater than zero. If we follow the code starting at line 44468, replacing op1 with 0, we can see that e2 equals 0 at line 44482, flags will be false and finally the e2 = -e2 operation will be executed generating the -0 result. I have implemented a testcase to reproduce the bug: https://paste.debian.net/1098800/ this code was compiled with -Ofast when we tested it. Should I file a bug about this? And for fixing, Is it a good solution to emit an instruction to return zero immediately if the input equals zero? > Jeff > Rafael Tsuha
[PATCH] Simplify sinh (x) / cosh (x)
This patch simplifies the expression sinh (x) / cosh (x) to tanh (x).
This rule is mathematically valid.
There's a slight difference in the result when applying this
optimization with x in the interval 0 < x <= 1e-4951. With the
optimization, the result using long double is -0 and without the
optimization, the result is +0.
When running the testsuite, the test gfortran.dg/pr79966.f90 failed,
but it seems unrelated to this patch
My architecture is x86_64.
gcc/ChangeLog
2019-01-22 Rafael Tsuha
* match.pd (sinh (x) / cosh (x)): New simplification rule.
gcc/testsuite/ChangeLog
2019-01-22 Rafael Tsuha
* gcc.dg/sinhovercosh-1.c: New test.
Index: gcc/match.pd
===
--- gcc/match.pd (revision 267671)
+++ gcc/match.pd (working copy)
@@ -4484,6 +4484,11 @@
(rdiv (SIN:s @0) (COS:s @0))
(TAN @0))
+ /* Simplify sinh(x) / cosh(x) -> tanh(x). */
+ (simplify
+ (rdiv (SINH:s @0) (COSH:s @0))
+ (TANH @0))
+
/* Simplify cos(x) / sin(x) -> 1 / tan(x). */
(simplify
(rdiv (COS:s @0) (SIN:s @0))
Index: gcc/testsuite/gcc.dg/sinhovercosh-1.c
===
--- gcc/testsuite/gcc.dg/sinhovercosh-1.c (nonexistent)
+++ gcc/testsuite/gcc.dg/sinhovercosh-1.c (working copy)
@@ -0,0 +1,45 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast -fdump-tree-optimized" } */
+
+extern float sinhf (float);
+extern float coshf (float);
+extern float tanhf (float);
+extern float sqrtf (float);
+extern double sinh (double);
+extern double cosh (double);
+extern double sqrt (double);
+extern double tanh (double);
+extern long double sinhl (long double);
+extern long double coshl (long double);
+extern long double tanhl (long double);
+extern long double sqrtl (long double);
+
+double __attribute__ ((noinline))
+sinhovercosh_ (double x)
+{
+ return sinh (x) / cosh (x);
+}
+
+float __attribute__ ((noinline))
+sinhfovercoshf_(float x)
+{
+ return sinhf (x) / coshf (x);
+}
+
+long double __attribute__ ((noinline))
+sinhlovercoshl_ (long double x)
+{
+ return sinhl (x) / coshl (x);
+}
+
+/* There must be no calls to sinh, cosh, or atanh */
+/* {dg-final { scan-tree-dump-not "sinh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "cosh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "sinfh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "cosfh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "sinlh " "optimized" } } */
+/* {dg-final { scan-tree-dump-not "coslh " "optimized" } } */
+/* {dg-final { scan-tree-dump-times "tanh " "1" "optimized" }} */
+/* {dg-final { scan-tree-dump-times "tanhl " "1" "optimized" }} */
+/* {dg-final { scan-tree-dump-times "tanhf " "1" "optimized" }} */
+
