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; 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 >