Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
Hi Richard,
I will investigate it further, but thanks again for your review!
Laurent
- Mail original -
> De: "Richard Biener"
> À: "Laurent Thevenoux"
> Cc: "GCC Patches"
> Envoyé: Lundi 9 Octobre 2017 15:57:38
> Objet: Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
>
> On Mon, Oct 9, 2017 at 3:30 PM, Laurent Thevenoux
> wrote:
> >
> >
> > - Mail original -
> >> De: "Richard Biener"
> >> À: "Laurent Thevenoux"
> >> Cc: "GCC Patches"
> >> Envoyé: Lundi 9 Octobre 2017 14:04:49
> >> Objet: Re: tree-complex.c: fix some_nonzerop test over reals (and a bug
> >> fix)
> >>
> >> On Fri, Oct 6, 2017 at 8:58 PM, Laurent Thevenoux
> >> wrote:
> >> > Hi Richard, thanks for your quick reply.
> >> >
> >> > - Mail original -
> >> >> De: "Richard Biener"
> >> >> À: "Laurent Thevenoux"
> >> >> Cc: "GCC Patches"
> >> >> Envoyé: Vendredi 6 Octobre 2017 13:42:57
> >> >> Objet: Re: tree-complex.c: fix some_nonzerop test over reals (and a bug
> >> >> fix)
> >> >>
> >> >> On Thu, Oct 5, 2017 at 4:41 PM, Laurent Thevenoux
> >> >> wrote:
> >> >> > Hello,
> >> >> >
> >> >> > This patch improves the some_nonzerop(tree t) function from
> >> >> > tree-complex.c
> >> >> > file (the function is only used there).
> >> >> >
> >> >> > This function returns true if a tree as a parameter is not the
> >> >> > constant
> >> >> > 0
> >> >> > (or +0.0 only for reals when !flag_signed_zeros ). The former result
> >> >> > is
> >> >> > then used to determine if some simplifications are possible for
> >> >> > complex
> >> >> > expansions (addition, multiplication, and division).
> >> >> >
> >> >> > Unfortunately, if the tree is a real constant, the function always
> >> >> > return
> >> >> > true, even for +0.0 because of the explicit test on flag_signed_zeros
> >> >> > (so
> >> >> > if your system enables signed zeros you cannot benefit from those
> >> >> > simplifications). To avoid this behavior and allow complex expansion
> >> >> > simplifications, I propose the following patch, which test for the
> >> >> > sign
> >> >> > of
> >> >> > the real constant 0.0 instead of checking the flag.
> >> >>
> >> >> But
> >> >>
> >> >> + if (TREE_CODE (t) == REAL_CST)
> >> >> +{
> >> >> + if (real_identical (&TREE_REAL_CST (t), &dconst0))
> >> >> + zerop = !real_isneg (&TREE_REAL_CST (t));
> >> >> +}
> >> >>
> >> >> shouldn't you do
> >> >>
> >> >>zerop = !flag_signed_zeros || !real_isneg (&TREE_REAL_CST (t));
> >> >>
> >> >> ?
> >> >
> >> > I’m not so sure. If I understand your proposition (tables below gives
> >> > values of zerop following the values of t and flag_signed_zeros):
> >> >
> >> >| zerop
> >> > t | !flag_signed_zeros is false | !flag_signed_zeros is true
> >> > -
> >> > +n | true* | true*
> >> > -n | false | true*
> >> > 0 | true| true
> >> > -0 | false | unreachable
> >> >
> >> > But here, results with a * don't return the good value. The existing
> >> > code
> >> > is also wrong, it always returns false if flag_signed_zeros is true,
> >> > else
> >> > the code is correct:
> >> >
> >> >| zerop
> >> > t | !flag_signed_zeros is false | !flag_signed_zeros is true
> >> > -
> >> > +n | false | false
> >> > -n | false | false
> >> > 0 | true| false*
> >> > -0 | false
Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
- Mail original -
> De: "Richard Biener"
> À: "Laurent Thevenoux"
> Cc: "GCC Patches"
> Envoyé: Lundi 9 Octobre 2017 14:04:49
> Objet: Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
>
> On Fri, Oct 6, 2017 at 8:58 PM, Laurent Thevenoux
> wrote:
> > Hi Richard, thanks for your quick reply.
> >
> > - Mail original -
> >> De: "Richard Biener"
> >> À: "Laurent Thevenoux"
> >> Cc: "GCC Patches"
> >> Envoyé: Vendredi 6 Octobre 2017 13:42:57
> >> Objet: Re: tree-complex.c: fix some_nonzerop test over reals (and a bug
> >> fix)
> >>
> >> On Thu, Oct 5, 2017 at 4:41 PM, Laurent Thevenoux
> >> wrote:
> >> > Hello,
> >> >
> >> > This patch improves the some_nonzerop(tree t) function from
> >> > tree-complex.c
> >> > file (the function is only used there).
> >> >
> >> > This function returns true if a tree as a parameter is not the constant
> >> > 0
> >> > (or +0.0 only for reals when !flag_signed_zeros ). The former result is
> >> > then used to determine if some simplifications are possible for complex
> >> > expansions (addition, multiplication, and division).
> >> >
> >> > Unfortunately, if the tree is a real constant, the function always
> >> > return
> >> > true, even for +0.0 because of the explicit test on flag_signed_zeros
> >> > (so
> >> > if your system enables signed zeros you cannot benefit from those
> >> > simplifications). To avoid this behavior and allow complex expansion
> >> > simplifications, I propose the following patch, which test for the sign
> >> > of
> >> > the real constant 0.0 instead of checking the flag.
> >>
> >> But
> >>
> >> + if (TREE_CODE (t) == REAL_CST)
> >> +{
> >> + if (real_identical (&TREE_REAL_CST (t), &dconst0))
> >> + zerop = !real_isneg (&TREE_REAL_CST (t));
> >> +}
> >>
> >> shouldn't you do
> >>
> >>zerop = !flag_signed_zeros || !real_isneg (&TREE_REAL_CST (t));
> >>
> >> ?
> >
> > I’m not so sure. If I understand your proposition (tables below gives
> > values of zerop following the values of t and flag_signed_zeros):
> >
> >| zerop
> > t | !flag_signed_zeros is false | !flag_signed_zeros is true
> > -
> > +n | true* | true*
> > -n | false | true*
> > 0 | true| true
> > -0 | false | unreachable
> >
> > But here, results with a * don't return the good value. The existing code
> > is also wrong, it always returns false if flag_signed_zeros is true, else
> > the code is correct:
> >
> >| zerop
> > t | !flag_signed_zeros is false | !flag_signed_zeros is true
> > -
> > +n | false | false
> > -n | false | false
> > 0 | true| false*
> > -0 | false | unreachable
> >
> > With the code I propose, we obtain the right results:
> >
> > t | zerop
> > --
> > +n | false
> > -n | false
> > 0 | true
> > -0 | false
> >
> > Do I really miss something (I'm sorry if I'm wrong)?
> >
> >>
> >> Also doesn't this miscalculate A - B for {ar, 0.} - {br, 0.} given the
> >> simplification simply
> >> returns bi?
> >
> > Here we are in case PAIR (ONLY_REAL, ONLY_REAL), which gives ri = 0. (ai)
> > even with signed zeros. So everything is OK.
> >
> >>
> >> So I'm not convinced this handling is correct.
> >>
> >> > This first fix reveals a bug (thanks to
> >> > c-c++-common/torture/complex-sign-add.c ) in the simplification section
> >> > of
> >> > expand_complex_addition (also fixed in the patch).
> >>
> >> Your patch looks backward and the existing code looks ok.
> >>
> >> @@ -937,7 +940,7 @@ expand_complex_addition (gimple_stmt_iterator
> >> *gsi, tree inner_type,
> >>
> >> case PAIR (VARYING, ONLY_REAL):
> >>rr = gimplify_build2 (gsi, code
Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
Hi Richard, thanks for your quick reply.
- Mail original -
> De: "Richard Biener"
> À: "Laurent Thevenoux"
> Cc: "GCC Patches"
> Envoyé: Vendredi 6 Octobre 2017 13:42:57
> Objet: Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
>
> On Thu, Oct 5, 2017 at 4:41 PM, Laurent Thevenoux
> wrote:
> > Hello,
> >
> > This patch improves the some_nonzerop(tree t) function from tree-complex.c
> > file (the function is only used there).
> >
> > This function returns true if a tree as a parameter is not the constant 0
> > (or +0.0 only for reals when !flag_signed_zeros ). The former result is
> > then used to determine if some simplifications are possible for complex
> > expansions (addition, multiplication, and division).
> >
> > Unfortunately, if the tree is a real constant, the function always return
> > true, even for +0.0 because of the explicit test on flag_signed_zeros (so
> > if your system enables signed zeros you cannot benefit from those
> > simplifications). To avoid this behavior and allow complex expansion
> > simplifications, I propose the following patch, which test for the sign of
> > the real constant 0.0 instead of checking the flag.
>
> But
>
> + if (TREE_CODE (t) == REAL_CST)
> +{
> + if (real_identical (&TREE_REAL_CST (t), &dconst0))
> + zerop = !real_isneg (&TREE_REAL_CST (t));
> +}
>
> shouldn't you do
>
>zerop = !flag_signed_zeros || !real_isneg (&TREE_REAL_CST (t));
>
> ?
I’m not so sure. If I understand your proposition (tables below gives values of
zerop following the values of t and flag_signed_zeros):
| zerop
t | !flag_signed_zeros is false | !flag_signed_zeros is true
-
+n | true* | true*
-n | false | true*
0 | true| true
-0 | false | unreachable
But here, results with a * don't return the good value. The existing code is
also wrong, it always returns false if flag_signed_zeros is true, else the code
is correct:
| zerop
t | !flag_signed_zeros is false | !flag_signed_zeros is true
-
+n | false | false
-n | false | false
0 | true| false*
-0 | false | unreachable
With the code I propose, we obtain the right results:
t | zerop
--
+n | false
-n | false
0 | true
-0 | false
Do I really miss something (I'm sorry if I'm wrong)?
>
> Also doesn't this miscalculate A - B for {ar, 0.} - {br, 0.} given the
> simplification simply
> returns bi?
Here we are in case PAIR (ONLY_REAL, ONLY_REAL), which gives ri = 0. (ai) even
with signed zeros. So everything is OK.
>
> So I'm not convinced this handling is correct.
>
> > This first fix reveals a bug (thanks to
> > c-c++-common/torture/complex-sign-add.c ) in the simplification section of
> > expand_complex_addition (also fixed in the patch).
>
> Your patch looks backward and the existing code looks ok.
>
> @@ -937,7 +940,7 @@ expand_complex_addition (gimple_stmt_iterator
> *gsi, tree inner_type,
>
> case PAIR (VARYING, ONLY_REAL):
>rr = gimplify_build2 (gsi, code, inner_type, ar, br);
> - ri = ai;
> + ri = bi;
>break;
With the existing code we don’t perform the simplification step at all since it
always returns (VARYING, VARYING) when flag_signed_zeros is true.
For instance, with {ar, -0.} + {br, 0.}, if simplification really occurs, its
in case PAIR (VARIYNG, ONLY_REAL) (because we consider -0. as a non_zero), and
it returns -0. if we return ai (instead of -0. + 0. -> 0.). But I understand
now that returning bi in this case is meaningless since {br, bi} is a ONLY_REAL
complex.
Nevertheless, for instance, {ar, -0.} + {br, 0.} -> {ar + br, -0.} is still
buggy.
A solution could be:
case PAIR (VARYING, ONLY_REAL):
rr = gimplify_build2 (gsi, code, inner_type, ar, br);
+ if (TREE_CODE (ai) == REAL_CST
+ && code = PLUS_EXPR
+ && real_identical (&TREE_REAL_CST (ai), &dconst0)
+ && real_isneg (&TREE_REAL_CST (ai)))
+ ri = bi;
+ else
ri = ai;
break;
Laurent
>
> down we have
>
> case PAIR (ONLY_REAL, VARYING):
> if (code == MINUS_EXPR)
> goto general;
> rr = gimplify_build2 (gsi, code, inner_type, ar, br);
> ri = bi;
> break;
>
> which for sure would need to change as well then. But for your
> changed code we know
> bi is zero (but ai may not).
>
> Richard.
>
> > The patch has passed bootstrap and testing on x86_64-pc-linux-gnu .
> >
> > Best regards,
> > Laurent Thévenoux
>
tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
Hello,
This patch improves the some_nonzerop(tree t) function from tree-complex.c file
(the function is only used there).
This function returns true if a tree as a parameter is not the constant 0 (or
+0.0 only for reals when !flag_signed_zeros ). The former result is then used
to determine if some simplifications are possible for complex expansions
(addition, multiplication, and division).
Unfortunately, if the tree is a real constant, the function always return true,
even for +0.0 because of the explicit test on flag_signed_zeros (so if your
system enables signed zeros you cannot benefit from those simplifications). To
avoid this behavior and allow complex expansion simplifications, I propose the
following patch, which test for the sign of the real constant 0.0 instead of
checking the flag.
This first fix reveals a bug (thanks to c-c++-common/torture/complex-sign-add.c
) in the simplification section of expand_complex_addition (also fixed in the
patch).
The patch has passed bootstrap and testing on x86_64-pc-linux-gnu .
Best regards,
Laurent Thévenoux
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 2191d62..a6124ce 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@
+2017-10-05 Laurent Thévenoux
+
+ * tree-complex.c (some_nonzerop): Adjust the way of testing real
+ constants. Allows existing simplifications of complex expansions
+ to be well done.
+ * tree-complex.c (expand_complex_addition): Bug fixing for complex
+ addition expansion simplification.
+
2017-10-02 Bill Schmidt
Backport from mainline
diff --git a/gcc/tree-complex.c b/gcc/tree-complex.c
index e0dd3d9..413b601 100644
--- a/gcc/tree-complex.c
+++ b/gcc/tree-complex.c
@@ -110,8 +110,11 @@ some_nonzerop (tree t)
/* Operations with real or imaginary part of a complex number zero
cannot be treated the same as operations with a real or imaginary
operand if we care about the signs of zeros in the result. */
- if (TREE_CODE (t) == REAL_CST && !flag_signed_zeros)
-zerop = real_identical (&TREE_REAL_CST (t), &dconst0);
+ if (TREE_CODE (t) == REAL_CST)
+{
+ if (real_identical (&TREE_REAL_CST (t), &dconst0))
+ zerop = !real_isneg (&TREE_REAL_CST (t));
+}
else if (TREE_CODE (t) == FIXED_CST)
zerop = fixed_zerop (t);
else if (TREE_CODE (t) == INTEGER_CST)
@@ -937,7 +940,7 @@ expand_complex_addition (gimple_stmt_iterator *gsi, tree inner_type,
case PAIR (VARYING, ONLY_REAL):
rr = gimplify_build2 (gsi, code, inner_type, ar, br);
- ri = ai;
+ ri = bi;
break;
case PAIR (VARYING, ONLY_IMAG):
