From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id E09F13858403 for ; Tue, 26 Oct 2021 13:13:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E09F13858403 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id D88E421954; Tue, 26 Oct 2021 13:13:29 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id B0CB8A3B8C; Tue, 26 Oct 2021 13:13:29 +0000 (UTC) Date: Tue, 26 Oct 2021 15:13:29 +0200 (CEST) From: Richard Biener To: Tamar Christina cc: "gcc-patches@gcc.gnu.org" , Jakub Jelinek , nd Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values In-Reply-To: Message-ID: References: <9o14q41-qs41-64qn-poo0-o29p3r98rr5s@fhfr.qr> <7s13204o-n6os-699-q544-65sr84rprnsq@fhfr.qr> MIME-Version: 1.0 X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Oct 2021 13:13:35 -0000 On Tue, 26 Oct 2021, Tamar Christina wrote: > > > > -----Original Message----- > > From: Richard Biener > > Sent: Tuesday, October 26, 2021 9:46 AM > > To: Tamar Christina > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd > > > > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns > > on signed values > > > > On Tue, 26 Oct 2021, Tamar Christina wrote: > > > > > > -----Original Message----- > > > > From: Richard Biener > > > > Sent: Tuesday, October 26, 2021 9:26 AM > > > > To: Tamar Christina > > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; nd > > > > > > > > Subject: RE: [PATCH] middle-end: fix de-optimizations with bitclear > > > > patterns on signed values > > > > > > > > On Mon, 25 Oct 2021, Tamar Christina wrote: > > > > > > > > > > -----Original Message----- > > > > > > From: Richard Biener > > > > > > Sent: Friday, October 15, 2021 12:31 PM > > > > > > To: Tamar Christina > > > > > > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek ; > > > > > > nd > > > > > > Subject: Re: [PATCH] middle-end: fix de-optimizations with > > > > > > bitclear patterns on signed values > > > > > > > > > > > > On Fri, 15 Oct 2021, Tamar Christina wrote: > > > > > > > > > > > > > Hi All, > > > > > > > > > > > > > > During testing after rebasing to commit I noticed a failing > > > > > > > testcase with the bitmask compare patch. > > > > > > > > > > > > > > Consider the following C++ testcase: > > > > > > > > > > > > > > #include > > > > > > > > > > > > > > #define A __attribute__((noipa)) A bool f5 (double i, double > > > > > > > j) { auto c = i <=> j; return c >= 0; } > > > > > > > > > > > > > > This turns into a comparison against chars, on systems where > > > > > > > chars are signed the pattern inserts an unsigned convert such > > > > > > > that it's able to do the transformation. > > > > > > > > > > > > > > i.e.: > > > > > > > > > > > > > > # RANGE [-1, 2] > > > > > > > # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > > > > # RANGE ~[3, 254] > > > > > > > _11 = (unsigned char) c$_M_value_22; > > > > > > > _19 = _11 <= 1; > > > > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > > > > D.10434 ={v} {CLOBBER}; > > > > > > > # .MEM_14 = VDEF <.MEM_24> > > > > > > > D.10407 ={v} {CLOBBER}; > > > > > > > # VUSE <.MEM_14> > > > > > > > return _19; > > > > > > > > > > > > > > instead of: > > > > > > > > > > > > > > # RANGE [-1, 2] > > > > > > > # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)> > > > > > > > # RANGE [-2, 2] > > > > > > > _3 = c$_M_value_5 & -2; > > > > > > > _19 = _3 == 0; > > > > > > > # .MEM_24 = VDEF <.MEM_6(D)> > > > > > > > D.10440 ={v} {CLOBBER}; > > > > > > > # .MEM_14 = VDEF <.MEM_24> > > > > > > > D.10413 ={v} {CLOBBER}; > > > > > > > # VUSE <.MEM_14> > > > > > > > return _19; > > > > > > > > > > > > > > This causes much worse codegen under -ffast-math due to phiops > > > > > > > no longer recognizing the pattern. It turns out that phiopts > > > > > > > spaceship_replacement is looking for the exact form that was > > > > > > > just > > > > changed. > > > > > > > > > > > > > > Trying to get it to recognize the new form is not trivial as > > > > > > > the transformation doesn't look to work when the thing it's > > > > > > > pointing to is itself > > > > > > a phi-node. > > > > > > > > > > > > What do you mean? Where it handles the BIT_AND it could also > > > > > > handle the conversion, no? The later handling would probably > > > > > > more explicitely need to distinguish between the BIT_AND and the > > > > > > conversion > > > > forms. > > > > > > > > > > Looks like I misunderstood the code, it was looking at the uses > > > > > not the defs of the value. > > > > > > > > > > --- inline copy of patch --- > > > > > > > > > > The comments seems to suggest this code only checks for (res & ~1) > > > > > == > > > > > 0 but the implementation seems to suggest it's broader. > > > > > > > > > > As such I added a case to check to see if the value comparison we > > > > > found is a type cast. and strips away the type cast and continues. > > > > > > > > > > In match.pd the typecasts are only added for signed comparisons to > > > > > == > > > > > 0 and != 0 which are then rewritten into comparisons with 1. > > > > > > > > > > As such I only check for 1 and LE and GT, which is what match.pd > > > > > would have rewritten it to. > > > > > > > > > > This fixes the regression but this is not code I 100% understand, > > > > > since I don't really know the semantics of the spaceship operator > > > > > so would appreciate an extra look. > > > > > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, > > > > > x86_64-pc-linux-gnu and no regressions. > > > > > > > > > > Ok for master? > > > > > > > > Please add a testcase. I hope Jakub can review the > > > > spaceship_replacement patch since he's the one familiar with the code. > > > > > > There's already a bunch of testcases that test the various variants: > > > gcc/testsuite/g++.dg/opt/pr94589-1.C > > > and gcc/testsuite/g++.dg/opt/pr94589-2.C which is how I noticed the > > > failure. However they only trigger the failure on signed chars. I > > > tried forcing `-fsigned-char` to see if I can make a general testcase but this > > seems to have not done it. > > > > > > Is there another flag I can use? > > > > I suppose you can copy the testcase(s) and replace 'auto' with 'signed char'? > > Unfortunately that dies with > > bit.cc: In function 'bool f5(double, double)': > bit.cc:4:52: error: cannot convert 'std::partial_ordering' to 'signed char' in initialization > 4 | A bool f5 (double i, double j) { signed char c = i <=> j; return c >= 0; } > | ~~^~~~~ > | | > | std::partial_ordering > > It looks like the chars end up there after inlining the `<=>` operation itself. > I could do a Gimple testcase, but worry it may be a bit fragile. try auto c = ...; signed char c2 = c; return c2 >= ... then > Regards, > Tamar. > > > > > > > > > Regards, > > > Tamar > > > > > > > > Thanks, > > > > Richard. > > > > > > > > > Thanks, > > > > > Tamar > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > * tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical > > > > > codegen. > > > > > > > > > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index > > > > > > > > > > > 0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78 > > > > aa2 > > > > > 340418571a33 100644 > > > > > --- a/gcc/tree-ssa-phiopt.c > > > > > +++ b/gcc/tree-ssa-phiopt.c > > > > > @@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, > > > > basic_block middle_bb, > > > > > tree lhs, rhs; > > > > > gimple *orig_use_stmt = use_stmt; > > > > > tree orig_use_lhs = NULL_TREE; > > > > > + bool is_canon = false; > > > > > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > > > > > if (is_gimple_assign (use_stmt) > > > > > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR @@ > > > > > -2063,6 > > > > > +2064,26 @@ spaceship_replacement (basic_block cond_bb, > > > > > +basic_block > > > > middle_bb, > > > > > } > > > > > else if (is_gimple_assign (use_stmt)) > > > > > { > > > > > + /* Deal with if match.pd has rewritten the (res & ~1) == 0 > > > > > + into res <= 1 and has left a type-cast for signed types. */ > > > > > + if (gimple_assign_cast_p (use_stmt)) > > > > > + { > > > > > + orig_use_lhs = gimple_assign_lhs (use_stmt); > > > > > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > > > > > + return false; > > > > > + if (EDGE_COUNT (phi_bb->preds) != 4) > > > > > + return false; > > > > > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > > > > > + return false; > > > > > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > > > > > + return false; > > > > > + tree_code cmp; > > > > > + if (is_gimple_assign (use_stmt) > > > > > + && (cmp = gimple_assign_rhs_code (use_stmt)) > > > > > + && (cmp == LE_EXPR || cmp == GT_EXPR) > > > > > + && wi::eq_p (wi::to_wide (gimple_assign_rhs2 (use_stmt)), 1)) > > > > > + is_canon = true; > > > > > + } > > > > > if (gimple_assign_rhs_class (use_stmt) == GIMPLE_BINARY_RHS) > > > > > { > > > > > cmp = gimple_assign_rhs_code (use_stmt); @@ -2099,7 +2120,9 > > > > @@ > > > > > spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > > > > > || !tree_fits_shwi_p (rhs) > > > > > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > > > > > return false; > > > > > - if (orig_use_lhs) > > > > > + /* If we're already in the canonical form we need to keep the original > > > > > + comparison. */ > > > > > + if (orig_use_lhs && !is_canon) > > > > > { > > > > > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > > > > > return false; > > > > > @@ -2310,6 +2333,7 @@ spaceship_replacement (basic_block cond_bb, > > > > basic_block middle_bb, > > > > > one_cmp = GT_EXPR; > > > > > > > > > > enum tree_code res_cmp; > > > > > + > > > > > switch (cmp) > > > > > { > > > > > case EQ_EXPR: > > > > > @@ -2345,6 +2369,8 @@ spaceship_replacement (basic_block cond_bb, > > > > basic_block middle_bb, > > > > > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > > > > > else if (integer_minus_onep (rhs)) > > > > > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > > > > > + else if (integer_onep (rhs) && is_canon) > > > > > + res_cmp = GE_EXPR; > > > > > else > > > > > return false; > > > > > break; > > > > > @@ -2353,6 +2379,8 @@ spaceship_replacement (basic_block cond_bb, > > > > basic_block middle_bb, > > > > > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > > > > > else if (integer_zerop (rhs)) > > > > > res_cmp = one_cmp; > > > > > + else if (integer_onep (rhs) && is_canon) > > > > > + res_cmp = LE_EXPR; > > > > > else > > > > > return false; > > > > > break; > > > > > > > > > > > > > -- > > > > Richard Biener > > > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 > > > > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg) > > > > > > > -- > > Richard Biener > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 > > Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg) > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)