diff mbox series

middle-end: check for vector mode before in get_mask_mode [PR116074]

Message ID patch-18681-tamar@arm.com
State New
Headers show
Series middle-end: check for vector mode before in get_mask_mode [PR116074] | expand

Commit Message

Tamar Christina July 26, 2024, 9:39 a.m. UTC
Hi All,

For historical reasons AArch64 has TI mode vector types but does not consider
TImode a vector mode.

What's happening in the PR is that get_vectype_for_scalar_type is returning
vector(1) TImode for a TImode scalar.  This then fails when we call
targetm.vectorize.get_mask_mode (vecmode).exists (&) on the TYPE_MODE.

I've checked other usages of get_mask_mode and none of them have anything that
would prevent this same issue from happening.  It only happens that normally
the vectorizer rejects the vector(1) type early, but in this case we get
further because the COND_EXPR hasn't been analyzed yet for a type.

I believe get_mask_mode shouldn't fault, and so this adds the check for vector
mode in the hook and returns nothing if it's not.  I did not add this to the
generic function because I believe this is an AArch64 quirk.

However the AArch64 maintainer does not agree, as such this fixes the PR.
I still don't think it's reasonable to have to force the caller to check
this.  And the target is doing something quite weird and unexpected to
future users of the hook.

Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.

Ok for master?

Thanks,
Tamar

gcc/ChangeLog:

	PR target/116074
	* tree-vect-patterns.cc (vect_recog_cond_store_pattern): Check vector mode.

gcc/testsuite/ChangeLog:

	PR target/116074
	* g++.target/aarch64/pr116074.C: New test.

---




--

Comments

Richard Biener July 26, 2024, 9:43 a.m. UTC | #1
> Am 26.07.2024 um 11:40 schrieb Tamar Christina <tamar.christina@arm.com>:
> 
> Hi All,
> 
> For historical reasons AArch64 has TI mode vector types but does not consider
> TImode a vector mode.
> 
> What's happening in the PR is that get_vectype_for_scalar_type is returning
> vector(1) TImode for a TImode scalar.  This then fails when we call
> targetm.vectorize.get_mask_mode (vecmode).exists (&) on the TYPE_MODE.
> 
> I've checked other usages of get_mask_mode and none of them have anything that
> would prevent this same issue from happening.  It only happens that normally
> the vectorizer rejects the vector(1) type early, but in this case we get
> further because the COND_EXPR hasn't been analyzed yet for a type.
> 
> I believe get_mask_mode shouldn't fault, and so this adds the check for vector
> mode in the hook and returns nothing if it's not.  I did not add this to the
> generic function because I believe this is an AArch64 quirk.
> 
> However the AArch64 maintainer does not agree, as such this fixes the PR.
> I still don't think it's reasonable to have to force the caller to check
> this.  And the target is doing something quite weird and unexpected to
> future users of the hook.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for master?

Ok

Richard 

> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
>    PR target/116074
>    * tree-vect-patterns.cc (vect_recog_cond_store_pattern): Check vector mode.
> 
> gcc/testsuite/ChangeLog:
> 
>    PR target/116074
>    * g++.target/aarch64/pr116074.C: New test.
> 
> ---
> diff --git a/gcc/testsuite/g++.target/aarch64/pr116074.C b/gcc/testsuite/g++.target/aarch64/pr116074.C
> new file mode 100644
> index 0000000000000000000000000000000000000000..54cf561510c460499a816ab6a84603fc20a5f1e5
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/aarch64/pr116074.C
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O3" } */
> +
> +int m[40];
> +
> +template <typename k> struct j {
> +  int length;
> +  k *e;
> +  void operator[](int) {
> +    if (length)
> +      __builtin___memcpy_chk(m, m+3, sizeof (k), -1);
> +  }
> +};
> +
> +j<j<int>> o;
> +
> +int *q;
> +
> +void ao(int i) {
> +  for (; i > 0; i--) {
> +    o[1];
> +    *q = 1;
> +  }
> +}
> diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc
> index 53af5e38b539159084846caac5da2ef4daaab4cb..82aeb8132a91e8cfc512a7ce9cb8be73ef28cab9 100644
> --- a/gcc/tree-vect-patterns.cc
> +++ b/gcc/tree-vect-patterns.cc
> @@ -6637,7 +6637,8 @@ vect_recog_cond_store_pattern (vec_info *vinfo,
> 
>   machine_mode mask_mode;
>   machine_mode vecmode = TYPE_MODE (vectype);
> -  if (targetm.vectorize.conditional_operation_is_expensive (IFN_MASK_STORE)
> +  if (!VECTOR_MODE_P (vecmode)
> +      || targetm.vectorize.conditional_operation_is_expensive (IFN_MASK_STORE)
>       || !targetm.vectorize.get_mask_mode (vecmode).exists (&mask_mode)
>       || !can_vec_mask_load_store_p (vecmode, mask_mode, false))
>     return NULL;
> 
> 
> 
> 
> --
> <rb18681.patch>
diff mbox series

Patch

diff --git a/gcc/testsuite/g++.target/aarch64/pr116074.C b/gcc/testsuite/g++.target/aarch64/pr116074.C
new file mode 100644
index 0000000000000000000000000000000000000000..54cf561510c460499a816ab6a84603fc20a5f1e5
--- /dev/null
+++ b/gcc/testsuite/g++.target/aarch64/pr116074.C
@@ -0,0 +1,24 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-O3" } */
+
+int m[40];
+
+template <typename k> struct j {
+  int length;
+  k *e;
+  void operator[](int) {
+    if (length)
+      __builtin___memcpy_chk(m, m+3, sizeof (k), -1);
+  }
+};
+
+j<j<int>> o;
+
+int *q;
+
+void ao(int i) {
+  for (; i > 0; i--) {
+    o[1];
+    *q = 1;
+  }
+}
diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc
index 53af5e38b539159084846caac5da2ef4daaab4cb..82aeb8132a91e8cfc512a7ce9cb8be73ef28cab9 100644
--- a/gcc/tree-vect-patterns.cc
+++ b/gcc/tree-vect-patterns.cc
@@ -6637,7 +6637,8 @@  vect_recog_cond_store_pattern (vec_info *vinfo,
 
   machine_mode mask_mode;
   machine_mode vecmode = TYPE_MODE (vectype);
-  if (targetm.vectorize.conditional_operation_is_expensive (IFN_MASK_STORE)
+  if (!VECTOR_MODE_P (vecmode)
+      || targetm.vectorize.conditional_operation_is_expensive (IFN_MASK_STORE)
       || !targetm.vectorize.get_mask_mode (vecmode).exists (&mask_mode)
       || !can_vec_mask_load_store_p (vecmode, mask_mode, false))
     return NULL;