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.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 (_REAL_CST (t), )) > >> >> + zerop = !real_isneg (_REAL_CST (t)); > >> >> +} > >> >> > >> >> shouldn't you do > >> >> > >> >>zerop = !flag_signed_zeros || !real_isneg (_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_ze
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 (_REAL_CST (t), )) >> >> + zerop = !real_isneg (_REAL_CST (t)); >> >> +} >> >> >> >> shouldn't you do >> >> >> >>zerop = !flag_signed_zeros || !real_isneg (_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
Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
- 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 (_REAL_CST (t), )) > >> + zerop = !real_isneg (_REAL_CST (t)); > >> +} > >> > >> shouldn't you do > >> > >>zerop = !flag_signed_zeros || !real_isneg (_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,
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 (_REAL_CST (t), )) >> + zerop = !real_isneg (_REAL_CST (t)); >> +} >> >> shouldn't you do >> >>zerop = !flag_signed_zeros || !real_isneg (_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): >
Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
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 (_REAL_CST (t), )) > + zerop = !real_isneg (_REAL_CST (t)); > +} > > shouldn't you do > >zerop = !flag_signed_zeros || !real_isneg (_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 (_REAL_CST (ai), ) + && real_isneg (_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 >
Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix)
On Thu, Oct 5, 2017 at 4:41 PM, Laurent Thevenouxwrote: > 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 (_REAL_CST (t), )) + zerop = !real_isneg (_REAL_CST (t)); +} shouldn't you do zerop = !flag_signed_zeros || !real_isneg (_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