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 08152385842E for ; Fri, 15 Oct 2021 15:19:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 08152385842E 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 AF5AF11D4; Fri, 15 Oct 2021 08:19:56 -0700 (PDT) Received: from localhost (unknown [10.32.98.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BB8943F66F; Fri, 15 Oct 2021 08:19:55 -0700 (PDT) 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 Lower intrinsics shift to GIMPLE when possible. References: Date: Fri, 15 Oct 2021 16:19:54 +0100 In-Reply-To: (Tamar Christina's message of "Fri, 15 Oct 2021 14:57:16 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_LOTSOFHASH, KAM_SHORT, 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 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: Fri, 15 Oct 2021 15:19:59 -0000 Tamar Christina writes: >> -----Original Message----- >> From: Richard Sandiford >> Sent: Friday, October 15, 2021 1:26 PM >> To: Tamar Christina >> Cc: gcc-patches@gcc.gnu.org; nd ; Richard Earnshaw >> ; Marcus Shawcroft >> ; Kyrylo Tkachov >> Subject: Re: [PATCH]AArch64 Lower intrinsics shift to GIMPLE when possib= le. >>=20 >> Tamar Christina writes: >> > Hi All, >> > >> > This lowers shifts to GIMPLE when the C interpretations of the shift >> > operations matches that of AArch64. >> > >> > In C shifting right by BITSIZE is undefined, but the behavior is >> > defined in AArch64. Additionally negative shifts lefts are undefined >> > in C but defined for the register variant of the instruction (SSHL, US= HL) as >> being right shifts. >> > >> > Since we have a right shift by immediate I rewrite those cases into >> > right shifts >> > >> > So: >> > >> > int64x1_t foo3 (int64x1_t a) >> > { >> > return vshl_s64 (a, vdup_n_s64(-6)); } >> > >> > produces: >> > >> > foo3: >> > sshr d0, d0, 6 >> > ret >> > >> > instead of: >> > >> > foo3: >> > mov x0, -6 >> > fmov d1, x0 >> > sshl d0, d0, d1 >> > ret >> > >> > This behavior isn't specifically mentioned for a left shift by >> > immediate, but I believe that only the case because we do have a right >> > shift by immediate but not a right shift by register. As such I do th= e same >> for left shift by immediate. >> > >> > The testsuite already has various testcases for shifts (vshl.c etc) so >> > I am not adding overlapping tests here. >> > >> > Out of range shifts like >> > >> > int64x1_t foo3 (int64x1_t a) >> > { >> > return vshl_s64 (a, vdup_n_s64(80)); } >> > >> > now get optimized to 0 as well along with undefined behaviors both in >> > C and AArch64. >>=20 >> The SSHL results are well-defined for all shift amounts, so we shouldn't >> convert them to undefined gimple, even as a temporary step. E.g.: >>=20 >> int32x4_t foo(int32x4_t x) { >> return vshlq_s32(x, vdupq_n_s32(256)); } >>=20 >> should fold to =E2=80=9Cx=E2=80=9D (if we fold it at all). Similarly: >>=20 >> int32x4_t foo(int32x4_t x) { >> return vshlq_s32(x, vdupq_n_s32(257)); } >>=20 >> should fold to x << 1 (again if we fold it at all). >>=20 >> For a shift right: >>=20 >> int32x4_t foo(int32x4_t x) { >> return vshlq_s32(x, vdupq_n_s32(-64)); } >>=20 >> is equivalent to: >>=20 >> int32x4_t foo(int32x4_t x) { >> return vshrq_n_s32(x, 31); >> } >>=20 >> and so it shouldn't fold to 0. > > And here I thought I had read the specs very carefully... > > I will punt on them because I don't think those ranged are common at all. Sounds good. There were other review comments further down the message (I should have been clearer about that, sorry). Could you have a look at those too? Thanks, Richard > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? > > --- inline copy of patch --- > > diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/a= arch64-builtins.c > index f6b41d9c200d6300dee65ba60ae94488231a8a38..568775cb8effaf51a692ba12a= f99e9865d2cf8a3 100644 > --- a/gcc/config/aarch64/aarch64-builtins.c > +++ b/gcc/config/aarch64/aarch64-builtins.c > @@ -2394,6 +2394,68 @@ aarch64_general_gimple_fold_builtin (unsigned int = fcode, gcall *stmt) > 1, args[0]); > gimple_call_set_lhs (new_stmt, gimple_call_lhs (stmt)); > break; > + BUILTIN_VSDQ_I_DI (BINOP, ashl, 3, NONE) > + { > + tree cst =3D args[1]; > + tree ctype =3D TREE_TYPE (cst); > + if (INTEGRAL_TYPE_P (ctype) > + && TREE_CODE (cst) =3D=3D INTEGER_CST) > + { > + wide_int wcst =3D wi::to_wide (cst); > + if (wi::neg_p (wcst, TYPE_SIGN (ctype))) > + new_stmt =3D > + gimple_build_assign (gimple_call_lhs (stmt), > + RSHIFT_EXPR, args[0], > + wide_int_to_tree (ctype, > + wi::abs (wcst))); > + else > + new_stmt =3D > + gimple_build_assign (gimple_call_lhs (stmt), > + LSHIFT_EXPR, args[0], args[1]); > + } > + } > + break; > + BUILTIN_VSDQ_I_DI (BINOP, sshl, 0, NONE) > + BUILTIN_VSDQ_I_DI (BINOP_UUS, ushl, 0, NONE) > + { > + tree cst =3D args[1]; > + tree ctype =3D TREE_TYPE (cst); > + HOST_WIDE_INT bits =3D GET_MODE_UNIT_BITSIZE (TYPE_MODE (TREE_TYPE (a= rgs[0]))); > + if (INTEGRAL_TYPE_P (ctype) > + && TREE_CODE (cst) =3D=3D INTEGER_CST) > + { > + wide_int wcst =3D wi::to_wide (cst); > + wide_int abs_cst =3D wi::abs (wcst); > + if (wi::geu_p (abs_cst, bits)) > + break; > + > + if (wi::neg_p (wcst, TYPE_SIGN (ctype))) > + new_stmt =3D > + gimple_build_assign (gimple_call_lhs (stmt), > + RSHIFT_EXPR, args[0], > + wide_int_to_tree (ctype, abs_cst)); > + else > + new_stmt =3D > + gimple_build_assign (gimple_call_lhs (stmt), > + LSHIFT_EXPR, args[0], args[1]); > + } > + } > + break; > + BUILTIN_VDQ_I (SHIFTIMM, ashr, 3, NONE) > + VAR1 (SHIFTIMM, ashr_simd, 0, NONE, di) > + BUILTIN_VDQ_I (USHIFTIMM, lshr, 3, NONE) > + VAR1 (USHIFTIMM, lshr_simd, 0, NONE, di) > + { > + tree cst =3D args[1]; > + tree ctype =3D TREE_TYPE (cst); > + HOST_WIDE_INT bits =3D GET_MODE_UNIT_BITSIZE (TYPE_MODE (TREE_TYPE (a= rgs[0]))); > + if (INTEGRAL_TYPE_P (ctype) > + && TREE_CODE (cst) =3D=3D INTEGER_CST > + && wi::ne_p (wi::to_wide (cst), bits)) > + new_stmt =3D gimple_build_assign (gimple_call_lhs (stmt), > + RSHIFT_EXPR, args[0], args[1]); > + } > + break; > BUILTIN_GPF (BINOP, fmulx, 0, ALL) > { > gcc_assert (nargs =3D=3D 2); > diff --git a/gcc/config/aarch64/aarch64-simd-builtins.def b/gcc/config/aa= rch64/aarch64-simd-builtins.def > index 402453aa9bba5949da43c984c4603196b1efd092..bbe0a4a3c4aea4187e7b1a9f1= 0ab60e79df7b138 100644 > --- a/gcc/config/aarch64/aarch64-simd-builtins.def > +++ b/gcc/config/aarch64/aarch64-simd-builtins.def > @@ -409,7 +409,7 @@ >=20=20 > BUILTIN_VDQ_I (SHIFTIMM, ashr, 3, NONE) > VAR1 (SHIFTIMM, ashr_simd, 0, NONE, di) > - BUILTIN_VDQ_I (SHIFTIMM, lshr, 3, NONE) > + BUILTIN_VDQ_I (USHIFTIMM, lshr, 3, NONE) > VAR1 (USHIFTIMM, lshr_simd, 0, NONE, di) > /* Implemented by aarch64_shr_n. */ > BUILTIN_VSDQ_I_DI (SHIFTIMM, srshr_n, 0, NONE) > diff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h > index 635a223b59eb0f64304351939d444411b697af81..c4ef5f7f7e3658c830893931e= f5a874842410e10 100644 > --- a/gcc/config/aarch64/arm_neon.h > +++ b/gcc/config/aarch64/arm_neon.h > @@ -27400,21 +27400,21 @@ __extension__ extern __inline uint8x8_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshr_n_u8 (uint8x8_t __a, const int __b) > { > - return (uint8x8_t) __builtin_aarch64_lshrv8qi ((int8x8_t) __a, __b); > + return __builtin_aarch64_lshrv8qi_uus (__a, __b); > } >=20=20 > __extension__ extern __inline uint16x4_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshr_n_u16 (uint16x4_t __a, const int __b) > { > - return (uint16x4_t) __builtin_aarch64_lshrv4hi ((int16x4_t) __a, __b); > + return __builtin_aarch64_lshrv4hi_uus (__a, __b); > } >=20=20 > __extension__ extern __inline uint32x2_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshr_n_u32 (uint32x2_t __a, const int __b) > { > - return (uint32x2_t) __builtin_aarch64_lshrv2si ((int32x2_t) __a, __b); > + return __builtin_aarch64_lshrv2si_uus (__a, __b); > } >=20=20 > __extension__ extern __inline uint64x1_t > @@ -27456,28 +27456,28 @@ __extension__ extern __inline uint8x16_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshrq_n_u8 (uint8x16_t __a, const int __b) > { > - return (uint8x16_t) __builtin_aarch64_lshrv16qi ((int8x16_t) __a, __b); > + return __builtin_aarch64_lshrv16qi_uus (__a, __b); > } >=20=20 > __extension__ extern __inline uint16x8_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshrq_n_u16 (uint16x8_t __a, const int __b) > { > - return (uint16x8_t) __builtin_aarch64_lshrv8hi ((int16x8_t) __a, __b); > + return __builtin_aarch64_lshrv8hi_uus (__a, __b); > } >=20=20 > __extension__ extern __inline uint32x4_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshrq_n_u32 (uint32x4_t __a, const int __b) > { > - return (uint32x4_t) __builtin_aarch64_lshrv4si ((int32x4_t) __a, __b); > + return __builtin_aarch64_lshrv4si_uus (__a, __b); > } >=20=20 > __extension__ extern __inline uint64x2_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshrq_n_u64 (uint64x2_t __a, const int __b) > { > - return (uint64x2_t) __builtin_aarch64_lshrv2di ((int64x2_t) __a, __b); > + return __builtin_aarch64_lshrv2di_uus (__a, __b); > } >=20=20 > __extension__ extern __inline int64_t > diff --git a/gcc/testsuite/gcc.target/aarch64/signbit-2.c b/gcc/testsuite= /gcc.target/aarch64/signbit-2.c > new file mode 100644 > index 0000000000000000000000000000000000000000..e4e9afc854317cb599fa8118a= 1117c5a52e6f497 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/signbit-2.c > @@ -0,0 +1,36 @@ > +/* { dg-do assemble } */ > +/* { dg-options "-O1 --save-temps" } */ > + > +#include > + > +int32x2_t foo1 (int32x2_t a) > +{ > + return vshr_n_s32 (vneg_s32 (a), 31); > +} > + > +int32x4_t foo2 (int32x4_t a) > +{ > + return vshrq_n_s32 (vnegq_s32 (a), 31); > +} > + > +int16x8_t foo3 (int16x8_t a) > +{ > + return vshrq_n_s16 (vnegq_s16 (a), 15); > +} > + > +int16x4_t foo4 (int16x4_t a) > +{ > + return vshr_n_s16 (vneg_s16 (a), 15); > +} > + > +int8x16_t foo5 (int8x16_t a) > +{ > + return vshrq_n_s8 (vnegq_s8 (a), 7); > +} > + > +int8x8_t foo6 (int8x8_t a) > +{ > + return vshr_n_s8 (vneg_s8 (a), 7); > +} > + > +/* { dg-final { scan-assembler-times {\tcmgt\t} 6 } } */