From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 2BD44388A824 for ; Mon, 25 May 2020 16:26:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2BD44388A824 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=segher@kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 04PGQbA3029616; Mon, 25 May 2020 11:26:37 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 04PGQaHq029615; Mon, 25 May 2020 11:26:36 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Mon, 25 May 2020 11:26:36 -0500 From: Segher Boessenkool To: "Yangfei (Felix)" Cc: "gcc-patches@gcc.gnu.org" , "Zhanghaijian (A)" Subject: Re: [PATCH PR94026] combine missed opportunity to simplify comparisons with zero Message-ID: <20200525162636.GE31009@gate.crashing.org> References: <20200318235137.GP22482@gate.crashing.org> <20200320013820.GE22482@gate.crashing.org> <20200323120938.GO22482@gate.crashing.org> <20200324145802.GR22482@gate.crashing.org> <20200523145727.GA31009@gate.crashing.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-18.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: Mon, 25 May 2020 16:26:49 -0000 On Mon, May 25, 2020 at 02:59:30AM +0000, Yangfei (Felix) wrote: > > It creates better code on all targets :-) A quite small improvement, but not > > entirely trivial. > > Thanks for the effort. It's great to hear that :- ) Yes :-) > > > > p.s. Please use a correct mime type? application/octet-stream > > > > isn't something I can reply to. Just text/plain is fine :-) > > > > > > I have using plain text now, hope that works for you. :-) > > > > Nope: > > > > [-- Attachment #2: pr94026-v2.diff --] > > [-- Type: application/octet-stream, Encoding: base64, Size: 5.9K --] > > This time I switched to use UUEncode type for the attachment. Does it work? No: [-- Attachment #2: pr94026-v3.diff --] [-- Type: application/octet-stream, Encoding: base64, Size: 5.8K --] > I am using Outlook and I didn't find the place to change the MIME type : - ( The simplest option is to use a different email client, one that plays nicely with others. You use git, maybe you could even use git-send-email? I'll paste things manually... > From a444419238c02c1e6ab9593a14a13e1e3dff90ed Mon Sep 17 00:00:00 2001 > From: Fei Yang > Date: Mon, 25 May 2020 10:19:30 +0800 > Subject: [PATCH] combine: missed opportunity to simplify comparisons with zero > [PR94026] (Capital "M" on "Missed" please) But, the subject should say what the patch *does*. So maybe combine: Simplify more comparisons with zero (PR94026) > If we have (and (lshiftrt X C) M) and M is a constant that would select > a field of bits within an item, but not the entire word, fold this into > a simple AND if we are in an equality comparison against zero. But that subject doesn't really describe what the patch does, anyway? > gcc/ > PR rtl-optimization/94026 > * combine.c (make_compound_operation_int): If we have (and > (lshiftrt X C) M) and M is a constant that would select a field > of bits within an item, but not the entire word, fold this into > a simple AND if we are in an equality comparison. > > gcc/testsuite/ > PR rtl-optimization/94026 > * gcc.dg/pr94026.c: New test. > --- a/gcc/ChangeLog > +++ b/gcc/ChangeLog > @@ -1,3 +1,11 @@ > +2020-05-25 Felix Yang > + > + PR rtl-optimization/94026 > + * combine.c (make_compound_operation_int): If we have (and > + (lshiftrt X C) M) and M is a constant that would select a field > + of bits within an item, but not the entire word, fold this into > + a simple AND if we are in an equality comparison. Don't put the changelog in the patch. > diff --git a/gcc/combine.c b/gcc/combine.c > index b044f29fd36..76d62b0bd17 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -8178,6 +8178,10 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr, > if (!CONST_INT_P (XEXP (x, 1))) > break; > > + HOST_WIDE_INT pos; > + unsigned HOST_WIDE_INT len; > + pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len); unsigned HOST_WIDE_INT len; HOST_WIDE_INT pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), &len); > @@ -8231,6 +8235,22 @@ make_compound_operation_int (scalar_int_mode mode, rtx *x_ptr, > new_rtx = make_compound_operation (new_rtx, in_code); > } > > + /* If we have (and (lshiftrt X C) M) and M is a constant that would select > + a field of bits within an item, but not the entire word, this might be > + representable by a simple AND if we are in an equality comparison. */ > + else if (pos > 0 && equality_comparison That "&& equality_comparison" should be on a separate line as well. > + && GET_CODE (XEXP (x, 0)) == LSHIFTRT > + && CONST_INT_P (XEXP (XEXP (x, 0), 1)) > + && pos + UINTVAL (XEXP (XEXP (x, 0), 1)) > + <= GET_MODE_BITSIZE (mode)) > + { > + new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code); > + HOST_WIDE_INT real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1)); > + unsigned HOST_WIDE_INT mask = ((unsigned HOST_WIDE_INT)1 << len) - 1; Space after cast. > + new_rtx = gen_rtx_AND (mode, new_rtx, > + gen_int_mode (mask << real_pos, mode)); > + } So this changes ((X >> C) & M) == ... to (X & (M << C)) == ... ? Where then does it check what ... is? This is only valid like this if that is zero. Why should this go in combine and not in simplify-rtx instead? > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr94026.c > @@ -0,0 +1,21 @@ > +/* { dg-do compile { target aarch64*-*-* i?86-*-* x86_64-*-* } } */ Why restrict this to only some targets? > +/* { dg-options "-O2 -fdump-rtl-combine" } */ > + > +int > +foo (int c) > +{ > + int a = (c >> 8) & 7; > + > + if (a >= 2) { > + return 1; > + } > + > + return 0; > +} > + > +/* The combine phase should transform (compare (and (lshiftrt x 8) 6) 0) > + to (compare (and (x 1536)) 0). We look for the *attempt* to match this > + RTL pattern, regardless of whether an actual insn may be found on the > + platform. */ > + > +/* { dg-final { scan-rtl-dump "\\(const_int 1536" "combine" } } */ That is a very fragile test. Segher