From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2023 invoked by alias); 26 Jun 2012 13:33:23 -0000 Received: (qmail 2015 invoked by uid 22791); 26 Jun 2012 13:33:21 -0000 X-SWARE-Spam-Status: No, hits=-4.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-gh0-f175.google.com (HELO mail-gh0-f175.google.com) (209.85.160.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 26 Jun 2012 13:32:56 +0000 Received: by ghbz2 with SMTP id z2so3898372ghb.20 for ; Tue, 26 Jun 2012 06:32:56 -0700 (PDT) MIME-Version: 1.0 Received: by 10.60.10.227 with SMTP id l3mr16219932oeb.39.1340717575966; Tue, 26 Jun 2012 06:32:55 -0700 (PDT) Received: by 10.76.82.4 with HTTP; Tue, 26 Jun 2012 06:32:55 -0700 (PDT) In-Reply-To: References: Date: Tue, 26 Jun 2012 13:44:00 -0000 Message-ID: Subject: Re: [Patch] PR 51938: extend ifcombine From: Richard Guenther To: Marc Glisse Cc: gcc-patches@gcc.gnu.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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 X-SW-Source: 2012-06/txt/msg01685.txt.bz2 On Sat, Jun 23, 2012 at 7:18 PM, Marc Glisse wrote: > Hello, > > thanks for looking into the patch. A couple more details now that I am ba= ck > from a conference: > > > On Wed, 20 Jun 2012, Marc Glisse wrote: > >> On Wed, 20 Jun 2012, Richard Guenther wrote: >> >>> On Sun, Jun 10, 2012 at 4:16 PM, Marc Glisse >>> wrote: >>>> >>>> Hello, >>>> >>>> currently, tree-ssa-ifcombine handles pairs of imbricated "if"s that >>>> share >>>> the same then branch, or the same else branch. There is no particular >>>> reason >>>> why it couldn't also handle the case where the then branch of one is t= he >>>> else branch of the other, which is what I do here. >>>> >>>> Any comments? >>> >>> >>> The general idea looks good, but I think the patch is too invasive. =A0= As >>> far >>> as I can see the only callers with a non-zero 'inv' argument come from >>> ifcombine_ifnotorif and ifcombine_ifnotandif (and both with inv =3D=3D = 2). > > > The idea of also supporting inv=3D=3D1 or inv=3D=3D3 is for uniformity, a= nd because > we might at some point want to get rid of the 'or' function and implement > everything in terms of the 'and' version, with suitably inverted tests. > > >>> I would rather see a more localized patch that makes use of >>> invert_tree_comparison to perform the inversion on the call arguments >>> of maybe_fold_and/or_comparisons. > > > I would rather have done that as well, and as a matter of fact that is wh= at > the first version of the patch did, until I realized that -ftrapping-math > was the default. > > >>> Is there any reason that would not work? >> >> >> invert_tree_comparison is useless for floating point (the case I am most >> interested in) unless we specify -fno-trapping-math (writing this patch >> taught me to add this flag to my default flags, but I can't expect every= one >> to do the same). An issue is that gcc mixes the behaviors of qnan and sn= an >> (it is not really an issue, it just means that !(comparison) can't be >> represented as comparison2). >> >>> At least >>> >>> + =A0if (inv & 1) >>> + =A0 =A0lcompcode2 =3D COMPCODE_TRUE - lcompcode2; >>> >>> looks as if it were not semantically correct - you cannot simply invert >>> floating-point comparisons (see the restrictions invert_tree_comparison >>> has). >> >> >> I don't remember all details, but I specifically thought of that, and the >> trapping behavior is handled a few lines below. > > > More specifically, it has (was already there, I didn't add it): > =A0 if (!honor_nans) > ... > =A0 else if (flag_trapping_math) > =A0 =A0 { > =A0 =A0 =A0 =A0/* Check that the original operation and the optimized one= s will trap > =A0 =A0 =A0 =A0 =A0 under the same condition. =A0*/ > =A0 =A0 =A0 =A0bool ltrap =3D (lcompcode & COMPCODE_UNORD) =3D=3D 0 > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 && (lcompcode !=3D COMPCODE_EQ) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 && (lcompcode !=3D COMPCODE_ORD); > ... same for rtrap and trap > > =A0 =A0 =A0 =A0/* In a short-circuited boolean expression the LHS might be > =A0 =A0 =A0 =A0 =A0 such that the RHS, if evaluated, will never trap. =A0= For > =A0 =A0 =A0 =A0 =A0 example, in ORD (x, y) && (x < y), we evaluate the RH= S only > =A0 =A0 =A0 =A0 =A0 if neither x nor y is NaN. =A0(This is a mixed blessi= ng: for > =A0 =A0 =A0 =A0 =A0 example, the expression above will never trap, hence > =A0 =A0 =A0 =A0 =A0 optimizing it to x < y would be invalid). =A0*/ > =A0 =A0 =A0 =A0if ((code =3D=3D TRUTH_ORIF_EXPR && (lcompcode & COMPCODE_= UNORD)) > =A0 =A0 =A0 =A0 =A0 =A0|| (code =3D=3D TRUTH_ANDIF_EXPR && !(lcompcode & = COMPCODE_UNORD))) > =A0 =A0 =A0 =A0 =A0rtrap =3D false; > > =A0 =A0 =A0 =A0/* If the comparison was short-circuited, and only the RHS > =A0 =A0 =A0 =A0 =A0 trapped, we may now generate a spurious trap. =A0*/ > =A0 =A0 =A0 =A0if (rtrap && !ltrap > =A0 =A0 =A0 =A0 =A0 =A0&& (code =3D=3D TRUTH_ANDIF_EXPR || code =3D=3D TR= UTH_ORIF_EXPR)) > =A0 =A0 =A0 =A0 =A0return NULL_TREE; > > =A0 =A0 =A0 =A0/* If we changed the conditions that cause a trap, we lose= . =A0*/ > =A0 =A0 =A0 =A0if ((ltrap || rtrap) !=3D trap) > ... > > which presumably ensures that the trapping behavior is preserved (I'll ha= ve > to double-check that I didn't break that logic). I was more concerned about the behavior with NaNs in general where x < y is not equal to x >=3D y. Now combine_comparison handles only the very specific case of logical and or or of two comparisons with the same operands, you basically make it handle && ~ and || ~, too (and ~ && ~ and ~ || ~), so it seems that optionally inverting the result of the 2nd operand should be enough making the interface prettier compared to the bitmask. With the following change this should be installed separately - it's a functional change already now /* If we changed the conditions that cause a trap, we lose. */ if ((ltrap || rtrap) !=3D trap) + { + /* Try the inverse condition. */ + compcode =3D COMPCODE_TRUE - compcode; + exchg =3D true; + trap =3D (compcode & COMPCODE_UNORD) =3D=3D 0 + && (compcode !=3D COMPCODE_EQ) + && (compcode !=3D COMPCODE_ORD); + } + if ((ltrap || rtrap) !=3D trap) return NULL_TREE; } ... + ret =3D fold_build2_loc (loc, tcode, truth_type, ll_arg, lr_arg); + if (exchg) + ret =3D fold_build1_loc (loc, TRUTH_NOT_EXPR, truth_type, ret); > Do you have an idea how this can be handled in a less intrusive way (and > without duplicating too much code)? Well, for one add an argument to ifcombine_iforif/ifandif whether the 2nd op is inverted and handle that appropriately instead of copying the functions. I'd go for a single bool argument for the inversion everywhere, too, both a= nd and or are commutative and you can handle all cases with that form. Thanks, Richard. > -- > Marc Glisse