Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)

[Laurent Thevenoux]

Hi Richard,

I will investigate it further, but thanks again for your review!

Laurent

- Mail original -
> De: "Richard Biener" <richard.guent...@gmail.com>
> À: "Laurent Thevenoux" <laurent.theven...@inria.fr>
> Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>
> 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
> <laurent.theven...@inria.fr> wrote:
> >
> >
> > - Mail original -
> >> De: "Richard Biener" <richard.guent...@gmail.com>
> >> À: "Laurent Thevenoux" <laurent.theven...@inria.fr>
> >> Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>
> >> 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
> >> <laurent.theven...@inria.fr> wrote:
> >> > Hi Richard, thanks for your quick reply.
> >> >
> >> > ----- Mail original -
> >> >> De: "Richard Biener" <richard.guent...@gmail.com>
> >> >> À: "Laurent Thevenoux" <laurent.theven...@inria.fr>
> >> >> Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>
> >> >> 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
> >> >> <laurent.theven...@inria.fr> 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 (_REAL_CST (t), ))
> >> >> +   zerop = !real_isneg (_REAL_CST (t));
> >> >> +}
> >> >>
> >> >> shouldn't you do
> >> >>
> >> >>zerop = !flag_signed_zeros || !real_isneg (_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_ze

Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)

[Richard Biener]

On Mon, Oct 9, 2017 at 3:30 PM, Laurent Thevenoux
<laurent.theven...@inria.fr> wrote:
>
>
> - Mail original -
>> De: "Richard Biener" <richard.guent...@gmail.com>
>> À: "Laurent Thevenoux" <laurent.theven...@inria.fr>
>> Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>
>> 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
>> <laurent.theven...@inria.fr> wrote:
>> > Hi Richard, thanks for your quick reply.
>> >
>> > - Mail original -
>> >> De: "Richard Biener" <richard.guent...@gmail.com>
>> >> À: "Laurent Thevenoux" <laurent.theven...@inria.fr>
>> >> Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>
>> >> 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
>> >> <laurent.theven...@inria.fr> 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 (_REAL_CST (t), ))
>> >> +   zerop = !real_isneg (_REAL_CST (t));
>> >> +}
>> >>
>> >> shouldn't you do
>> >>
>> >>zerop = !flag_signed_zeros || !real_isneg (_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 

Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)

[Laurent Thevenoux]

- Mail original -
> De: "Richard Biener" <richard.guent...@gmail.com>
> À: "Laurent Thevenoux" <laurent.theven...@inria.fr>
> Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>
> 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
> <laurent.theven...@inria.fr> wrote:
> > Hi Richard, thanks for your quick reply.
> >
> > - Mail original -
> >> De: "Richard Biener" <richard.guent...@gmail.com>
> >> À: "Laurent Thevenoux" <laurent.theven...@inria.fr>
> >> Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>
> >> 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
> >> <laurent.theven...@inria.fr> 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 (_REAL_CST (t), ))
> >> +   zerop = !real_isneg (_REAL_CST (t));
> >> +}
> >>
> >> shouldn't you do
> >>
> >>zerop = !flag_signed_zeros || !real_isneg (_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,

Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)

[Richard Biener]

On Fri, Oct 6, 2017 at 8:58 PM, Laurent Thevenoux
<laurent.theven...@inria.fr> wrote:
> Hi Richard, thanks for your quick reply.
>
> - Mail original -
>> De: "Richard Biener" <richard.guent...@gmail.com>
>> À: "Laurent Thevenoux" <laurent.theven...@inria.fr>
>> Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>
>> 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
>> <laurent.theven...@inria.fr> 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 (_REAL_CST (t), ))
>> +   zerop = !real_isneg (_REAL_CST (t));
>> +}
>>
>> shouldn't you do
>>
>>zerop = !flag_signed_zeros || !real_isneg (_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):
>

Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)

[Laurent Thevenoux]

Hi Richard, thanks for your quick reply.

- Mail original -
> De: "Richard Biener" <richard.guent...@gmail.com>
> À: "Laurent Thevenoux" <laurent.theven...@inria.fr>
> Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>
> 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
> <laurent.theven...@inria.fr> 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 (_REAL_CST (t), ))
> +   zerop = !real_isneg (_REAL_CST (t));
> +}
> 
> shouldn't you do
> 
>zerop = !flag_signed_zeros || !real_isneg (_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 (_REAL_CST (ai), )
+  && real_isneg (_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
>

Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)

[Richard Biener]

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 (_REAL_CST (t), ))
+   zerop = !real_isneg (_REAL_CST (t));
+}

shouldn't you do

   zerop = !flag_signed_zeros || !real_isneg (_REAL_CST (t));

?

Also doesn't this miscalculate A - B for {ar, 0.} - {br, 0.} given the
simplification simply
returns bi?

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;

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