public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Tamar Christina <tamar.christina@arm.com>
To: gcc-patches@gcc.gnu.org
Cc: nd@arm.com, Richard.Earnshaw@arm.com, Marcus.Shawcroft@arm.com,
	Kyrylo.Tkachov@arm.com, richard.sandiford@arm.com
Subject: [PATCH]AArch64  Fix vector re-interpretation between partial SIMD modes
Date: Fri, 11 Nov 2022 14:45:42 +0000	[thread overview]
Message-ID: <patch-16562-tamar@arm.com> (raw)

[-- Attachment #1: Type: text/plain, Size: 3018 bytes --]

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..84dbe2f4ea7d03b424602ed98a34e7824217dc91 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));
+
+      /* 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;
+    }
 
   if (maybe_ne (BITS_PER_SVE_VECTOR, 128u))
     {




-- 

[-- Attachment #2: rb16562.patch --]
[-- Type: text/plain, Size: 1929 bytes --]

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index d3c3650d7d728f56adb65154127dc7b72386c5a7..84dbe2f4ea7d03b424602ed98a34e7824217dc91 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));
+
+      /* 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;
+    }
 
   if (maybe_ne (BITS_PER_SVE_VECTOR, 128u))
     {




             reply	other threads:[~2022-11-11 14:45 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-11 14:45 Tamar Christina [this message]
2022-11-17 21:13 ` Richard Sandiford
2022-11-18  9:29   ` Richard Sandiford
2022-12-01 16:20     ` Tamar Christina
2022-12-05 11:41       ` Richard Sandiford

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=patch-16562-tamar@arm.com \
    --to=tamar.christina@arm.com \
    --cc=Kyrylo.Tkachov@arm.com \
    --cc=Marcus.Shawcroft@arm.com \
    --cc=Richard.Earnshaw@arm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=nd@arm.com \
    --cc=richard.sandiford@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).