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 E4B1638187D6 for ; Mon, 5 Dec 2022 11:41:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E4B1638187D6 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 3382D23A; Mon, 5 Dec 2022 03:41:13 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.99.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 919CE3F73D; Mon, 5 Dec 2022 03:41:05 -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 Fix vector re-interpretation between partial SIMD modes References: Date: Mon, 05 Dec 2022 11:41:04 +0000 In-Reply-To: (Tamar Christina's message of "Thu, 1 Dec 2022 16:20: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=-39.2 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,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: >> -----Original Message----- >> From: Richard Sandiford >> Sent: Friday, November 18, 2022 9:30 AM >> To: Tamar Christina >> Cc: gcc-patches@gcc.gnu.org; nd ; Richard Earnshaw >> ; Marcus Shawcroft >> ; Kyrylo Tkachov >> Subject: Re: [PATCH]AArch64 Fix vector re-interpretation between partial >> SIMD modes >> >> Richard Sandiford via Gcc-patches writes: >> > Tamar Christina writes: >> >> Hi All, >> >> >> >> While writing a patch series I started getting incorrect codegen out >> >> from VEC_PERM on partial struct types. >> >> >> >> It turns out that this was happening because the >> >> TARGET_CAN_CHANGE_MODE_CLASS implementation has a slight bug in >> it. The hook only checked for SIMD to >> >> Partial but never Partial to SIMD. This resulted in incorrect subregs to be >> >> generated from the fallback code in VEC_PERM_EXPR expansions. >> >> >> >> I have unfortunately not been able to trigger it using a standalone >> >> testcase as the mid-end optimizes away the permute every time I try >> >> to describe a permute that would result in the bug. >> >> >> >> The patch now rejects any conversion of partial SIMD struct types, >> >> unless they are both partial structures of the same number of >> >> registers or one is a SIMD type who's size is less than 8 bytes. >> >> >> >> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. >> >> >> >> Ok for master? And backport to GCC 12? >> >> >> >> Thanks, >> >> Tamar >> >> >> >> gcc/ChangeLog: >> >> >> >> * config/aarch64/aarch64.cc (aarch64_can_change_mode_class): >> Restrict >> >> conversions between partial struct types properly. >> >> >> >> --- inline copy of patch -- >> >> diff --git a/gcc/config/aarch64/aarch64.cc >> >> b/gcc/config/aarch64/aarch64.cc index >> >> >> d3c3650d7d728f56adb65154127dc7b72386c5a7..84dbe2f4ea7d03b424602ed9 >> 8a3 >> >> 4e7824217dc91 100644 >> >> --- a/gcc/config/aarch64/aarch64.cc >> >> +++ b/gcc/config/aarch64/aarch64.cc >> >> @@ -26471,9 +26471,10 @@ aarch64_can_change_mode_class >> (machine_mode from, >> >> bool from_pred_p = (from_flags & VEC_SVE_PRED); >> >> bool to_pred_p = (to_flags & VEC_SVE_PRED); >> >> >> >> - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | >> VEC_STRUCT)); >> >> bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | >> VEC_STRUCT >> >> | VEC_PARTIAL)); >> >> + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD >> | VEC_STRUCT >> >> + | VEC_PARTIAL)); >> >> >> >> /* Don't allow changes between predicate modes and other modes. >> >> Only predicate registers can hold predicate modes and only @@ >> >> -26496,9 +26497,23 @@ aarch64_can_change_mode_class >> (machine_mode from, >> >> return false; >> >> >> >> /* Don't allow changes between partial and full Advanced SIMD >> structure >> >> - modes. */ >> >> - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) >> >> - return false; >> >> + modes unless both are a partial struct with the same number of >> registers >> >> + or the vector bitsizes must be the same. */ >> >> + if (to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) >> >> + { >> >> + /* If they're both partial structures, allow if they have the same >> number >> >> + or registers. */ >> >> + if (to_partial_advsimd_struct_p == from_partial_advsimd_struct_p) >> >> + return known_eq (GET_MODE_SIZE (from), GET_MODE_SIZE (to)); >> > >> > It looks like the ^ makes this line unreachable. I guess it should be >> > a separate top-level condition. >> > >> >> + /* If one is a normal SIMD register, allow only if no larger than 64-bit. >> */ >> >> + if ((to_flags & VEC_ADVSIMD) == to_flags) >> >> + return known_le (GET_MODE_SIZE (to), 8); >> >> + else if ((from_flags & VEC_ADVSIMD) == from_flags) >> >> + return known_le (GET_MODE_SIZE (from), 8); >> >> + >> >> + return false; >> >> + } >> > >> > I don't think we need to restrict this to SIMD modes. A plain DI >> > would be OK too. So I think it should just be: >> > >> > return (known_le (GET_MODE_SIZE (to), 8) >> > || known_le (GET_MODE_SIZE (from, 8)); >> >> Looking again, all the other tests return false if they found a definite problem >> and fall through to later code otherwise. I think we should do the same here. > > I've rewritten the conditions. I needed to allow any conversions as long as they're both > partial vectors. There were various intrinsics tests that rely on for instance being able to > take a subreg of a VN2x8QI from a VN3x8QI for instance. I did not only allow the smaller > case since it didn't seem logical to block paradoxical subregs of this kind. > > Reload seems to be correctly handling them as separate 64-bit registers (we have tests for > this it looks like.) > > So Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? And backport to GCC 12? OK for both, thanks. Richard > Thanks, > Tamar > > gcc/ChangeLog: > > * config/aarch64/aarch64.cc (aarch64_can_change_mode_class): Restrict > conversions between partial struct types properly. > > ---- inline copy of patch --- > > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index c91df6f5006c257690aafb75398933d628a970e1..ff02c78f895b26a70b2653e58db453f4b5870666 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -26658,9 +26658,10 @@ aarch64_can_change_mode_class (machine_mode from, > bool from_pred_p = (from_flags & VEC_SVE_PRED); > bool to_pred_p = (to_flags & VEC_SVE_PRED); > > - bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT)); > bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | VEC_STRUCT > | VEC_PARTIAL)); > + bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT > + | VEC_PARTIAL)); > > /* Don't allow changes between predicate modes and other modes. > Only predicate registers can hold predicate modes and only > @@ -26682,9 +26683,10 @@ aarch64_can_change_mode_class (machine_mode from, > || GET_MODE_UNIT_SIZE (from) != GET_MODE_UNIT_SIZE (to))) > return false; > > - /* Don't allow changes between partial and full Advanced SIMD structure > - modes. */ > - if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p) > + /* Don't allow changes between partial and other registers only if > + one is a normal SIMD register, allow only if not larger than 64-bit. */ > + if ((to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p) > + && (known_gt (GET_MODE_SIZE (to), 8) || known_gt (GET_MODE_SIZE (to), 8))) > return false; > > if (maybe_ne (BITS_PER_SVE_VECTOR, 128u))