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)
Hi Richard,
I will investigate it further, but thanks again for your review!
Laurent
----- 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 15:57:38
> Objet: 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
> >> >>
> >>
>