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

[Laurent Thevenoux <laurent dot thevenoux at inria dot fr>]

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;

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
>