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 5CA333858D20 for ; Fri, 27 Jan 2023 09:54:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5CA333858D20 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 D9D7D2B; Fri, 27 Jan 2023 01:55:07 -0800 (PST) Received: from [10.57.76.247] (unknown [10.57.76.247]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6A7143F5A1; Fri, 27 Jan 2023 01:54:25 -0800 (PST) Message-ID: Date: Fri, 27 Jan 2023 09:54:19 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR 107674] Content-Language: en-US To: Kyrylo Tkachov , "gcc-patches@gcc.gnu.org" Cc: Richard Earnshaw References: <13d03aef-f5d1-03fe-5281-31921d24dce0@arm.com> From: "Andre Vieira (lists)" In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-17.2 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,NICE_REPLY_A,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: On 26/01/2023 15:02, Kyrylo Tkachov wrote: > Hi Andre, > >> -----Original Message----- >> From: Andre Vieira (lists) >> Sent: Tuesday, January 24, 2023 1:41 PM >> To: gcc-patches@gcc.gnu.org >> Cc: Kyrylo Tkachov ; Richard Earnshaw >> >> Subject: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR >> 107674] >> >> Hi, >> >> The ACLE defines mve_pred16_t as an unsigned short. This patch makes >> sure GCC treats the predicate as an unsigned type, rather than signed. >> >> Bootstrapped on aarch64-none-eabi and regression tested on arm-none-eabi >> and armeb-none-eabi for armv8.1-m.main+mve.fp. >> >> OK for trunk? >> >> gcc/ChangeLog: >> >> PR target/107674 >> * config/arm/arm-builtins.cc (arm_simd_builtin_type): Rewrite to >> use >> new qualifiers parameter and use unsigned short type for MVE >> predicate. >> (arm_init_builtin): Call arm_simd_builtin_type with qualifiers >> parameter. >> (arm_init_crypto_builtins): Likewise. >> >> gcc/testsuite/ChangeLog: >> >> PR target/107674 >> * gcc.target/arm/mve/mve_vpt.c: New test. > > diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-builtins.cc > index 11d7478d9df69139802a9d42c09dd0de7480b60e..6c67cec93ff76a4b42f3a0b305f697142e88fcd9 100644 > --- a/gcc/config/arm/arm-builtins.cc > +++ b/gcc/config/arm/arm-builtins.cc > @@ -1489,12 +1489,14 @@ arm_lookup_simd_builtin_type (machine_mode mode, > } > > static tree > -arm_simd_builtin_type (machine_mode mode, bool unsigned_p, bool poly_p) > +arm_simd_builtin_type (machine_mode mode, enum arm_type_qualifiers qualifiers) > { > > I think in C++ now we can leave out the "enum" here. > > diff --git a/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c > new file mode 100644 > index 0000000000000000000000000000000000000000..26a565b79dd1348e361b3aa23a1d6e6d13bffce8 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c > @@ -0,0 +1,27 @@ > +/* { dg-options "-O2" } */ > +/* { dg-require-effective-target arm_v8_1m_mve_ok } */ > +/* { dg-add-options arm_v8_1m_mve } */ > +/* { dg-final { check-function-bodies "**" "" } } */ > +#include > +void test0 (uint8_t *a, uint8_t *b, uint8_t *c) > +{ > + uint8x16_t va = vldrbq_u8 (a); > + uint8x16_t vb = vldrbq_u8 (b); > + mve_pred16_t p = vcmpeqq_u8 (va, vb); > + uint8x16_t vc = vaddq_x_u8 (va, vb, p); > + vstrbq_p_u8 (c, vc, p); > +} > +/* > +** test0: > +** vldrb.8 q2, \[r0\] > +** vldrb.8 q1, \[r1\] > +** vcmp.i8 eq, q2, q1 > +** vmrs r3, p0 @ movhi > +** uxth r3, r3 > +** vmsr p0, r3 @ movhi > +** vpst > +** vaddt.i8 q3, q2, q1 > +** vpst > +** vstrbt.8 q3, \[r2\] > +** bx lr > +*/ > > This explicit assembly matching looks quite fragile and sensitive to future scheduling and RA changes. > Is there something more targeted we could scan for to check that the predicate is unsigned now? No not really, as it's not unsigned everywhere, only in its intermediate representation between intrinsics. GCC is aware that mve_pred16_t is an unsigned short, so as soon as you try to use the value on its own or pass it as an argument or return, there is an implicit cast. I could make this particular test-case more robust by not checking specific registers. Though the sequence of loads-cmp-add-store will always be the same as it's data-dependent.