From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 15AF7385841D for ; Tue, 2 May 2023 12:22:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 15AF7385841D Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DDD1BC14; Tue, 2 May 2023 05:23:17 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3B0B73F64C; Tue, 2 May 2023 05:22:33 -0700 (PDT) From: Richard Sandiford To: Andrew Pinski via Gcc-patches Mail-Followup-To: Andrew Pinski via Gcc-patches ,Andrew Pinski , richard.sandiford@arm.com Cc: Andrew Pinski Subject: Re: [PATCH] target: [PR109657] (a ? -1 : 0) | b could be optimized better for aarch64 References: <20230428233446.688570-1-apinski@marvell.com> Date: Tue, 02 May 2023 13:22:32 +0100 In-Reply-To: <20230428233446.688570-1-apinski@marvell.com> (Andrew Pinski via Gcc-patches's message of "Fri, 28 Apr 2023 16:34:46 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-30.0 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_NUMSUBJECT,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Andrew Pinski via Gcc-patches writes: > There is no canonical form for this case defined. So the aarch64 backend needs > a pattern to match both of these forms. > > The forms are: > (set (reg/i:SI 0 x0) > (if_then_else:SI (eq (reg:CC 66 cc) > (const_int 0 [0])) > (reg:SI 97) > (const_int -1 [0xffffffffffffffff]))) > and > (set (reg/i:SI 0 x0) > (ior:SI (neg:SI (ne:SI (reg:CC 66 cc) > (const_int 0 [0]))) > (reg:SI 102))) > > Currently the aarch64 backend matches the first form so this > patch adds a insn_and_split to match the second form and > convert it to the first form. > > OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions > > PR target/109657 > > gcc/ChangeLog: > > * config/aarch64/aarch64.md (*cmov_insn_m1): New > insn_and_split pattern. > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/csinv-2.c: New test. > --- > gcc/config/aarch64/aarch64.md | 20 +++++++++++++++++ > gcc/testsuite/gcc.target/aarch64/csinv-2.c | 26 ++++++++++++++++++++++ > 2 files changed, 46 insertions(+) > create mode 100644 gcc/testsuite/gcc.target/aarch64/csinv-2.c > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index e1a2b265b20..57fe5601350 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -4194,6 +4194,26 @@ (define_insn "*cmovsi_insn_uxtw" > [(set_attr "type" "csel, csel, csel, csel, csel, mov_imm, mov_imm")] > ) > > +;; There are two canonical forms for `cmp ? -1 : a`. > +;; This is the second form and is here to help combine. > +;; Support `-(cmp) | a` into `cmp ? -1 : a` to be canonical in the backend. > +(define_insn_and_split "*cmov_insn_m1" > + [(set (match_operand:GPI 0 "register_operand" "=r") > + (ior:GPI > + (neg:GPI > + (match_operator:GPI 1 "aarch64_comparison_operator" > + [(match_operand 2 "cc_register" "") (const_int 0)])) > + (match_operand 3 "register_operand" "r")))] > + "" > + "#" > + "&& true" > + [(set (match_dup 0) > + (if_then_else:GPI (match_dup 1) > + (const_int -1) (match_dup 3)))] Sorry for the nit, but the formatting of the last two lines looks odd IMO. How about: (if_then_else:GPI (match_dup 1) (const_int -1) (match_dup 3))... or: (if_then_else:GPI (match_dup 1) (const_int -1) (match_dup 3))... OK with that change, thanks. Richard > + {} > + [(set_attr "type" "csel")] > +) > + > (define_insn "*cmovdi_insn_uxtw" > [(set (match_operand:DI 0 "register_operand" "=r") > (if_then_else:DI > diff --git a/gcc/testsuite/gcc.target/aarch64/csinv-2.c b/gcc/testsuite/gcc.target/aarch64/csinv-2.c > new file mode 100644 > index 00000000000..89132acb713 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/csinv-2.c > @@ -0,0 +1,26 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > +/* PR target/109657: (a ? -1 : 0) | b could be better */ > + > +/* Both functions should have the same assembly of: > + cmp w1, 0 > + csinv w0, w0, wzr, eq > + > + We should not get: > + cmp w1, 0 > + csetm w1, ne > + orr w0, w1, w0 > + */ > +/* { dg-final { scan-assembler-times "csinv\tw\[0-9\]" 2 } } */ > +/* { dg-final { scan-assembler-not "csetm\tw\[0-9\]" } } */ > +unsigned b(unsigned a, unsigned b) > +{ > + if(b) > + return -1; > + return a; > +} > +unsigned b1(unsigned a, unsigned b) > +{ > + unsigned t = b ? -1 : 0; > + return a | t; > +}