This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
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.thevenoux@inria.fr> wrote:
>
>
> ----- 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.
But the problem is in your change and not in the code you try to fix
it. Testing coverage seems weak given your fix didn't
regress anything tho.
>>
>> 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?
Well, not off-hand but your testing showed one issue at least. For
{ar, -0.} + {br, 0.} with VARYING, ONLY_REAL you may not simply use ai
as result. See
fold_real_zero_addition_p ().
Richard.
>>
>> 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
>> >>
>>