From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119930 invoked by alias); 9 Oct 2017 12:05:00 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 117289 invoked by uid 89); 9 Oct 2017 12:04:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.3 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=convinced, envoy, octobre, Best X-HELO: mail-wm0-f50.google.com Received: from mail-wm0-f50.google.com (HELO mail-wm0-f50.google.com) (74.125.82.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Oct 2017 12:04:51 +0000 Received: by mail-wm0-f50.google.com with SMTP id q124so21909718wmb.0 for ; Mon, 09 Oct 2017 05:04:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=zE1I6rXafjXM6is8g09xjMztI/ZtecJhUDxRVHFQ4kE=; b=cTQxJwLc8QfefncWtRWRla4MHjTdaIu8ZK1bP9zpGkIs8dQXnhfZxOx9xnJ6H54hOm ZRlNZEXSIRay5qJqclcehOVvQaSpCS27SKqC21V2XPqm1pjdJvDz7qm76wuPg67SvWby XfuvR2+7/cIPntNYnQSN7BbeCxTWOSmO9dBkheQoaSz13msPrwjOtc1srKrn0xYh4nMB iD+lySRDPxmYDow2InUOdhlji14yf2XSph7PMaO5gw8py3RYnRj3UgimFyIaErNz2atn nvwyKVIjrSqk1xIUS5S17MPIG2h4/Q6QKDyorN/1pxBq7oU7yvevw21n3l0bBAbtwTIs rOuw== X-Gm-Message-State: AMCzsaWejA/qdrtRjeyhUyT4gI2jhJOwgpCrlQTGorajppAHVrjblvd6 hsO0tOzIIouKApGDzMELHJonbhfWcryHcil283VjEA== X-Google-Smtp-Source: AOwi7QDq/HAV96xC1WLUlljnEsU06POjrjv88NtlAbDUlPGgqLZquEXX4fOYjraK/D85Z9V+LKs2+tOPtuswbMlxI5k= X-Received: by 10.80.175.165 with SMTP id h34mr13659963edd.292.1507550689788; Mon, 09 Oct 2017 05:04:49 -0700 (PDT) MIME-Version: 1.0 Received: by 10.80.143.34 with HTTP; Mon, 9 Oct 2017 05:04:49 -0700 (PDT) In-Reply-To: <1145206242.13837197.1507316295816.JavaMail.zimbra@inria.fr> References: <518273718.13144082.1507214398559.JavaMail.zimbra@inria.fr> <241978471.13145496.1507214474104.JavaMail.zimbra@inria.fr> <1145206242.13837197.1507316295816.JavaMail.zimbra@inria.fr> From: Richard Biener Date: Mon, 09 Oct 2017 12:30:00 -0000 Message-ID: Subject: Re: tree-complex.c: fix some_nonzerop test over reals (and a bug fix) To: Laurent Thevenoux Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00472.txt.bz2 On Fri, Oct 6, 2017 at 8:58 PM, Laurent Thevenoux wrote: > Hi Richard, thanks for your quick reply. > > ----- Mail original ----- >> De: "Richard Biener" >> =C3=80: "Laurent Thevenoux" >> Cc: "GCC Patches" >> Envoy=C3=A9: 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 >> wrote: >> > Hello, >> > >> > This patch improves the some_nonzerop(tree t) function from tree-compl= ex.c >> > file (the function is only used there). >> > >> > This function returns true if a tree as a parameter is not the constan= t 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 ret= urn >> > 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 sig= n of >> > the real constant 0.0 instead of checking the flag. >> >> But >> >> + if (TREE_CODE (t) =3D=3D REAL_CST) >> + { >> + if (real_identical (&TREE_REAL_CST (t), &dconst0)) >> + zerop =3D !real_isneg (&TREE_REAL_CST (t)); >> + } >> >> shouldn't you do >> >> zerop =3D !flag_signed_zeros || !real_isneg (&TREE_REAL_CST (t)); >> >> ? > > I=E2=80=99m not so sure. If I understand your proposition (tables below g= ives 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 =3D 0. (a= i) 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 sectio= n 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 =3D gimplify_build2 (gsi, code, inner_type, ar, br); >> - ri =3D ai; >> + ri =3D bi; >> break; > > With the existing code we don=E2=80=99t perform the simplification step a= t 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_ze= ro), 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 sti= ll buggy. > > A solution could be: > > case PAIR (VARYING, ONLY_REAL): > rr =3D gimplify_build2 (gsi, code, inner_type, ar, br); > + if (TREE_CODE (ai) =3D=3D REAL_CST > + && code =3D PLUS_EXPR > + && real_identical (&TREE_REAL_CST (ai), &dconst0) > + && real_isneg (&TREE_REAL_CST (ai))) > + ri =3D bi; > + else > ri =3D 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 =3D=3D MINUS_EXPR) >> goto general; >> rr =3D gimplify_build2 (gsi, code, inner_type, ar, br); >> ri =3D 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=C3=A9venoux >>