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

[Laurent Thevenoux]

----- Mail original -----
> De: "Richard Biener" <richard.guenther@gmail.com>
> À: "Laurent Thevenoux" <laurent.thevenoux@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.thevenoux@inria.fr> wrote:
> > Hi Richard, thanks for your quick reply.
> >
> > ----- Mail original -----
> >> De: "Richard Biener" <richard.guenther@gmail.com>
> >> À: "Laurent Thevenoux" <laurent.thevenoux@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.thevenoux@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 (&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;
> 
> I still don't understand what bug you are fixing.

I'm trying to write some improvements for the complex arithmetic support, and, for this purpose I need to detect some zeros.
So I firstly thought that some_nonzerop must return false for +0. (with or without signed zeros).

> 
> I think you are fixing fallout of your some_nonzero change in a
> strange way which shows your change isn't correct.

The testsuite revealed it after my change in some_nonzerop. Then, only the case {ar, -0.} + {br, 0.} failed, while all the others have succeed.

> 
> All the simplifications in expand_complex_addition (and probably
> elsewhere) do _not_ properly handle
> signed zeros.  So returning true from some_nonzerp for
> flag_signed_zeros is dangerous, even if it _is_ +0.0.

OK, so its maybe this part that I don’t understand. Why it is dangerous to return false from some_nonzerop for +0.0 only?
Is it a matter of some intermediate negations which wouldn't be detected by some_nonzerop?
Do you know an example of simplification which is not properly handled in that case?

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