Hi Richard, I will investigate it further, but thanks again for your review!
Laurent ----- Mail original ----- > De: "Richard Biener" <richard.guent...@gmail.com> > À: "Laurent Thevenoux" <laurent.theven...@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.theven...@inria.fr> wrote: > > > > > > ----- Mail original ----- > >> De: "Richard Biener" <richard.guent...@gmail.com> > >> À: "Laurent Thevenoux" <laurent.theven...@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.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'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 > >> >> > >> >