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 32D13388B6AB for ; Tue, 15 Nov 2022 10:36:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 32D13388B6AB 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 34A261477; Tue, 15 Nov 2022 02:36:32 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.62]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 19C983F73B; Tue, 15 Nov 2022 02:36:24 -0800 (PST) From: Richard Sandiford To: Tamar Christina Mail-Followup-To: Tamar Christina ,"gcc-patches\@gcc.gnu.org" , Richard Earnshaw , nd , Marcus Shawcroft , richard.sandiford@arm.com Cc: "gcc-patches\@gcc.gnu.org" , Richard Earnshaw , nd , Marcus Shawcroft Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. References: Date: Tue, 15 Nov 2022 10:36:23 +0000 In-Reply-To: (Tamar Christina's message of "Mon, 14 Nov 2022 15:58:38 +0000") 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=-41.2 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP 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: Tamar Christina writes: > Hello, > > Ping and updated patch. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > * config/aarch64/aarch64.md (*tb1): Rename to... > (*tb1): ... this. > (tbranch4): New. > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/tbz_1.c: New test. > > --- inline copy of patch --- > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 2bc2684b82c35a44e0a2cea6e3aaf32d939f8cdf..d7684c93fba5b717d568e1a4fd712bde55c7c72e 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -943,12 +943,29 @@ (define_insn "*cb1" > (const_int 1)))] > ) > > -(define_insn "*tb1" > +(define_expand "tbranch4" > [(set (pc) (if_then_else > - (EQL (zero_extract:DI (match_operand:GPI 0 "register_operand" "r") > - (const_int 1) > - (match_operand 1 > - "aarch64_simd_shift_imm_" "n")) > + (match_operator 0 "aarch64_comparison_operator" > + [(match_operand:ALLI 1 "register_operand") > + (match_operand:ALLI 2 "aarch64_simd_shift_imm_")]) > + (label_ref (match_operand 3 "" "")) > + (pc)))] > + "optimize > 0" Why's the pattern conditional on optimize? Seems a valid choice at -O0 too. I think the split here shows the difficulty with having a single optab and a comparison operator though. operand 0 can be something like: (eq x 1) but we're not comparing x for equality with 1. We're testing whether bit 1 is zero. This means that operand 0 can't be taken literally and can't be used directly in insn patterns. In an earlier review, I'd said: For the TB instructions (and for other similar instructions that I've seen on other architectures) it would be more useful to have a single-bit test, with operand 4 specifying the bit position. Arguably it might then be better to have separate eq and ne optabs, to avoid the awkward doubling of the operands (operand 1 contains operands 2 and 3). I think we should do that eq/ne split (sorry for not pushing harder for it before). Thanks, Richard > +{ > + rtx bitvalue = gen_reg_rtx (DImode); > + rtx tmp = simplify_gen_subreg (DImode, operands[1], GET_MODE (operands[1]), 0); > + emit_insn (gen_extzv (bitvalue, tmp, const1_rtx, operands[2])); > + operands[2] = const0_rtx; > + operands[1] = aarch64_gen_compare_reg (GET_CODE (operands[0]), bitvalue, > + operands[2]); > +}) > + > +(define_insn "*tb1" > + [(set (pc) (if_then_else > + (EQL (zero_extract:GPI (match_operand:ALLI 0 "register_operand" "r") > + (const_int 1) > + (match_operand 1 > + "aarch64_simd_shift_imm_" "n")) > (const_int 0)) > (label_ref (match_operand 2 "" "")) > (pc))) > @@ -959,15 +976,15 @@ (define_insn "*tb1" > { > if (get_attr_far_branch (insn) == 1) > return aarch64_gen_far_branch (operands, 2, "Ltb", > - "\\t%0, %1, "); > + "\\t%0, %1, "); > else > { > operands[1] = GEN_INT (HOST_WIDE_INT_1U << UINTVAL (operands[1])); > - return "tst\t%0, %1\;\t%l2"; > + return "tst\t%0, %1\;\t%l2"; > } > } > else > - return "\t%0, %1, %l2"; > + return "\t%0, %1, %l2"; > } > [(set_attr "type" "branch") > (set (attr "length") > diff --git a/gcc/testsuite/gcc.target/aarch64/tbz_1.c b/gcc/testsuite/gcc.target/aarch64/tbz_1.c > new file mode 100644 > index 0000000000000000000000000000000000000000..86f5d3e23cf7f1ea6f3596549ce1a0cff6774463 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/tbz_1.c > @@ -0,0 +1,95 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-O2 -std=c99 -fno-unwind-tables -fno-asynchronous-unwind-tables" } */ > +/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */ > + > +#include > + > +void h(void); > + > +/* > +** g1: > +** tbnz x[0-9]+, #?0, .L([0-9]+) > +** ret > +** ... > +*/ > +void g1(bool x) > +{ > + if (__builtin_expect (x, 0)) > + h (); > +} > + > +/* > +** g2: > +** tbz x[0-9]+, #?0, .L([0-9]+) > +** b h > +** ... > +*/ > +void g2(bool x) > +{ > + if (__builtin_expect (x, 1)) > + h (); > +} > + > +/* > +** g3_ge: > +** tbnz w[0-9]+, #?31, .L[0-9]+ > +** b h > +** ... > +*/ > +void g3_ge(int x) > +{ > + if (__builtin_expect (x >= 0, 1)) > + h (); > +} > + > +/* > +** g3_gt: > +** cmp w[0-9]+, 0 > +** ble .L[0-9]+ > +** b h > +** ... > +*/ > +void g3_gt(int x) > +{ > + if (__builtin_expect (x > 0, 1)) > + h (); > +} > + > +/* > +** g3_lt: > +** tbz w[0-9]+, #?31, .L[0-9]+ > +** b h > +** ... > +*/ > +void g3_lt(int x) > +{ > + if (__builtin_expect (x < 0, 1)) > + h (); > +} > + > +/* > +** g3_le: > +** cmp w[0-9]+, 0 > +** bgt .L[0-9]+ > +** b h > +** ... > +*/ > +void g3_le(int x) > +{ > + if (__builtin_expect (x <= 0, 1)) > + h (); > +} > + > +/* > +** g5: > +** mov w[0-9]+, 65279 > +** tst w[0-9]+, w[0-9]+ > +** beq .L[0-9]+ > +** b h > +** ... > +*/ > +void g5(int x) > +{ > + if (__builtin_expect (x & 0xfeff, 1)) > + h (); > +}