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

[Richard Biener <richard dot guenther at gmail dot com>]

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 think you are fixing fallout of your some_nonzero change in a
strange way which shows your change isn't correct.

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.

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