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