| Message ID | 241978471.13145496.1507214474104.JavaMail.zimbra@inria.fr |
|---|---|
| State | New |
| Headers | show |
| Series | tree-complex.c: fix some_nonzerop test over reals (and a bug fix) | expand |
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)); ? Also doesn't this miscalculate A - B for {ar, 0.} - {br, 0.} given the simplification simply returns bi? 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; 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
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; 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 >
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 >>
----- 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. > > 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? > > 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 > >> >
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 >> >> >>
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 > >> >> > >> >
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 2191d62..a6124ce 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,11 @@ +2017-10-05 Laurent Thévenoux <laurent.thevenoux@inria.fr> + + * tree-complex.c (some_nonzerop): Adjust the way of testing real + constants. Allows existing simplifications of complex expansions + to be well done. + * tree-complex.c (expand_complex_addition): Bug fixing for complex + addition expansion simplification. + 2017-10-02 Bill Schmidt <wschmidt@linux.vnet.ibm.com> Backport from mainline diff --git a/gcc/tree-complex.c b/gcc/tree-complex.c index e0dd3d9..413b601 100644 --- a/gcc/tree-complex.c +++ b/gcc/tree-complex.c @@ -110,8 +110,11 @@ some_nonzerop (tree t) /* Operations with real or imaginary part of a complex number zero cannot be treated the same as operations with a real or imaginary operand if we care about the signs of zeros in the result. */ - if (TREE_CODE (t) == REAL_CST && !flag_signed_zeros) - zerop = real_identical (&TREE_REAL_CST (t), &dconst0); + if (TREE_CODE (t) == REAL_CST) + { + if (real_identical (&TREE_REAL_CST (t), &dconst0)) + zerop = !real_isneg (&TREE_REAL_CST (t)); + } else if (TREE_CODE (t) == FIXED_CST) zerop = fixed_zerop (t); else if (TREE_CODE (t) == INTEGER_CST) @@ -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; case PAIR (VARYING, ONLY_IMAG):