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 CBE423835791 for ; Tue, 15 Nov 2022 11:10:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CBE423835791 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 AFF4E13D5; Tue, 15 Nov 2022 03:10:41 -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 79B7C3F73B; Tue, 15 Nov 2022 03:10:34 -0800 (PST) From: Richard Sandiford To: Tamar Christina Mail-Followup-To: Tamar Christina ,"gcc-patches\@gcc.gnu.org" , nd , Richard Earnshaw , Marcus Shawcroft , Kyrylo Tkachov , richard.sandiford@arm.com Cc: "gcc-patches\@gcc.gnu.org" , nd , Richard Earnshaw , Marcus Shawcroft , Kyrylo Tkachov Subject: Re: [PATCH]AArch64 Extend umov and sbfx patterns. References: Date: Tue, 15 Nov 2022 11:10:33 +0000 In-Reply-To: (Tamar Christina's message of "Fri, 11 Nov 2022 14:42:45 +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=-40.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,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: > Hi, > >> > --- a/gcc/config/aarch64/aarch64-simd.md >> > +++ b/gcc/config/aarch64/aarch64-simd.md >> > @@ -4259,7 +4259,7 @@ (define_insn >> "*aarch64_get_lane_zero_extend" >> > ;; Extracting lane zero is split into a simple move when it is >> > between SIMD ;; registers or a store. >> > (define_insn_and_split "aarch64_get_lane" >> > - [(set (match_operand: 0 "aarch64_simd_nonimmediate_operand" >> > "=?r, w, Utv") >> > + [(set (match_operand: 0 "aarch64_simd_nonimmediate_operand" >> > + "=r, w, Utv") >> > (vec_select: >> > (match_operand:VALL_F16_FULL 1 "register_operand" "w, w, w") >> > (parallel [(match_operand:SI 2 "immediate_operand" "i, i, i")])))] >> >> Which testcase does this help with? It didn't look like the new tests do any >> vector stuff. >> > > Right, sorry about that, splitting up my patches resulted in this sneaking in from a different series. > Moved now. > >> > -(define_insn "*_ashl" >> > +(define_insn "*_ashl" >> > [(set (match_operand:GPI 0 "register_operand" "=r") >> > (ANY_EXTEND:GPI >> > - (ashift:SHORT (match_operand:SHORT 1 "register_operand" "r") >> > + (ashift:ALLX (match_operand:ALLX 1 "register_operand" "r") >> > (match_operand 2 "const_int_operand" "n"))))] >> > - "UINTVAL (operands[2]) < GET_MODE_BITSIZE (mode)" >> > + "UINTVAL (operands[2]) < GET_MODE_BITSIZE (mode)" >> >> It'd be better to avoid even defining si<-si or si<-di "extensions" >> (even though nothing should try to match them), so how about adding: >> >> > && >> >> or similar to the beginning of the condition? The conditions for the invalid >> combos will then be provably false at compile time and the patterns will be >> compiled out. >> > > Done. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > * config/aarch64/aarch64.md > (*_ashl): Renamed to... > (*_ashl): ...this. > (*zero_extend_lshr): Renamed to... > (*zero_extend_lshr): ...this. > (*extend_ashr): Rename to... > (*extend_ashr): ...this. > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/bitmove_1.c: New test. > * gcc.target/aarch64/bitmove_2.c: New test. > > --- inline copy of patch --- > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index d7684c93fba5b717d568e1a4fd712bde55c7c72e..d230bbb833f97813c8371aa07b587bd8b0292cee 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -5711,40 +5711,43 @@ (define_insn "*extrsi5_insn_di" > [(set_attr "type" "rotate_imm")] > ) > > -(define_insn "*_ashl" > +(define_insn "*_ashl" > [(set (match_operand:GPI 0 "register_operand" "=r") > (ANY_EXTEND:GPI > - (ashift:SHORT (match_operand:SHORT 1 "register_operand" "r") > + (ashift:ALLX (match_operand:ALLX 1 "register_operand" "r") > (match_operand 2 "const_int_operand" "n"))))] > - "UINTVAL (operands[2]) < GET_MODE_BITSIZE (mode)" > + " > > + && UINTVAL (operands[2]) < GET_MODE_BITSIZE (mode)" > { > - operands[3] = GEN_INT ( - UINTVAL (operands[2])); > + operands[3] = GEN_INT ( - UINTVAL (operands[2])); > return "bfiz\t%0, %1, %2, %3"; > } > [(set_attr "type" "bfx")] > ) > > -(define_insn "*zero_extend_lshr" > +(define_insn "*zero_extend_lshr" > [(set (match_operand:GPI 0 "register_operand" "=r") > (zero_extend:GPI > - (lshiftrt:SHORT (match_operand:SHORT 1 "register_operand" "r") > - (match_operand 2 "const_int_operand" "n"))))] > - "UINTVAL (operands[2]) < GET_MODE_BITSIZE (mode)" > + (lshiftrt:ALLX (match_operand:ALLX 1 "register_operand" "r") > + (match_operand 2 "const_int_operand" "n"))))] > + " > > + && UINTVAL (operands[2]) < GET_MODE_BITSIZE (mode)" > { > - operands[3] = GEN_INT ( - UINTVAL (operands[2])); > + operands[3] = GEN_INT ( - UINTVAL (operands[2])); > return "ubfx\t%0, %1, %2, %3"; > } > [(set_attr "type" "bfx")] > ) > > -(define_insn "*extend_ashr" > +(define_insn "*extend_ashr" > [(set (match_operand:GPI 0 "register_operand" "=r") > (sign_extend:GPI > - (ashiftrt:SHORT (match_operand:SHORT 1 "register_operand" "r") > - (match_operand 2 "const_int_operand" "n"))))] > - "UINTVAL (operands[2]) < GET_MODE_BITSIZE (mode)" > + (ashiftrt:ALLX (match_operand:ALLX 1 "register_operand" "r") > + (match_operand 2 "const_int_operand" "n"))))] > + " > > + && UINTVAL (operands[2]) < GET_MODE_BITSIZE (mode)" > { > - operands[3] = GEN_INT ( - UINTVAL (operands[2])); > + operands[3] = GEN_INT ( - UINTVAL (operands[2])); > return "sbfx\\t%0, %1, %2, %3"; > } > [(set_attr "type" "bfx")] > diff --git a/gcc/testsuite/gcc.target/aarch64/bitmove_1.c b/gcc/testsuite/gcc.target/aarch64/bitmove_1.c > new file mode 100644 > index 0000000000000000000000000000000000000000..5ea4265f55213d7e7e5193a3a3681c9350867b50 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/bitmove_1.c > @@ -0,0 +1,76 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-O3 -std=c99" } */ > +/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */ > + > +#include > + > +/* > +** sfoo6: > +** asr x0, x0, 16 > +** ret > +*/ > +int64_t sfoo6 (int32_t x) > +{ > + return x >> 16; > +} > + > +/* > +** ufoo6: > +** lsr w0, w0, 30 > +** ret > +*/ > +uint64_t ufoo6 (uint32_t x) > +{ > + return x >> 30; > +} > + > +/* > +** ufoo6s: > +** ubfx w0, w0, 7, 9 > +** ret > +*/ > +uint32_t ufoo6s (uint16_t x) > +{ > + return x >> 7; > +} > + > +/* > +** ufoo6h: > +** ubfx w0, w0, 4, 4 > +** ret > +*/ > +uint16_t ufoo6h (uint8_t x) > +{ > + return x >> 4; > +} > + > +/* > +** sfoo62: > +** sbfx x0, x0, 10, 22 > +** ret > +*/ > +int64_t sfoo62 (int32_t x) > +{ > + return x >> 10; > +} > + > +/* > +** ufoo62: > +** lsr w0, w0, 10 > +** ret > +*/ > +uint64_t ufoo62 (uint32_t x) > +{ > + return x >> 10; > +} > + > +/* > +** sfoo63: > +** sbfx x0, x0, 10, 22 > +** ret > +*/ > +int64_t sfoo63 (int32_t x) > +{ > + return x >> 10; > +} This is the same as sfoo62, not sure if that's intentional. > + > diff --git a/gcc/testsuite/gcc.target/aarch64/bitmove_2.c b/gcc/testsuite/gcc.target/aarch64/bitmove_2.c > new file mode 100644 > index 0000000000000000000000000000000000000000..329600cb3dbecf4cdfed994f6cfdf98ab77e8a01 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/bitmove_2.c > @@ -0,0 +1,76 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-O3 -std=c99" } */ > +/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */ > + > +#include > + > +/* > +** sfoo6: > +** sbfiz x0, x0, 16, 16 > +** ret > +*/ > +int64_t sfoo6 (int32_t x) > +{ > + return x << 16; > +} > + > +/* > +** ufoo6: > +** lsl w0, w0, 30 > +** ret > +*/ > +uint64_t ufoo6 (uint32_t x) > +{ > + return x << 30; > +} > + > +/* > +** ufoo6s: > +** ubfiz w0, w0, 7, 16 > +** ret > +*/ > +uint32_t ufoo6s (uint16_t x) > +{ > + return x << 7; > +} > + > +/* > +** ufoo6h: > +** ... > +** ubfiz w0, w0, 4, 12 > +** ret > +*/ > +uint16_t ufoo6h (uint8_t x) > +{ > + return x << 4; > +} This looks odd without the ... filled in. It raises the question why the width is 12 bits when the original type was only 8. > + > +/* > +** sfoo62: > +** sbfiz x0, x0, 10, 22 > +** ret > +*/ > +int64_t sfoo62 (int32_t x) > +{ > + return x << 10; > +} > + > +/* > +** ufoo62: > +** lsl w0, w0, 10 > +** ret > +*/ > +uint64_t ufoo62 (uint32_t x) > +{ > + return x << 10; > +} > + > +/* > +** sfoo63: > +** sbfiz x0, x0, 10, 22 > +** ret > +*/ > +int64_t sfoo63 (int32_t x) > +{ > + return x << 10; > +} Similarly a dup of sfoo62. OK with those things fixed, thanks. Richard