diff mbox series

c-family: Tighten vector handling in type_for_mode [PR94072]

Message ID mpta74c4z2t.fsf@arm.com
State New
Headers show
Series c-family: Tighten vector handling in type_for_mode [PR94072] | expand

Commit Message

Richard Sandiford March 19, 2020, 6:51 a.m. UTC
In this PR we had a 512-bit VECTOR_TYPE whose mode is XImode
(an integer mode used for four 128-bit vectors).  When trying
to expand a zero constant for it, we hit code in expand_expr_real_1
that tries to use the associated integer type instead.  The code used
type_for_mode (XImode, 1) to get this integer type.

However, the c-family implementation of type_for_mode checks for
any registered built-in type that matches the mode and has the
right signedness.  This meant that it could return a built-in
vector type when given an integer mode (particularly if, as here,
the vector type isn't supported by the current subtarget and so
TYPE_MODE != TYPE_MODE_RAW).  The expand code would then cycle
endlessly trying to use this "new" type instead of the original
vector type.

The search loop is probably too lax in other ways -- e.g. it could
return records that just happen to have the right mode -- but this
seems like a safe, incremental improvement.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Richard


2020-03-18  Richard Sandiford  <richard.sandiford@arm.com>

gcc/c-family/
	PR middle-end/94072
	* c-common.c (c_common_type_for_mode): Before using a registered
	built-in type, check that the vectorness of the type matches
	the vectorness of the mode.

gcc/testsuite/
	PR middle-end/94072
	* gcc.target/aarch64/pr94072.c: New test.
---
 gcc/c-family/c-common.c                    | 11 +++++++----
 gcc/testsuite/gcc.target/aarch64/pr94072.c |  9 +++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr94072.c

Comments

Jeff Law via Gcc-patches March 19, 2020, 12:53 p.m. UTC | #1
On Thu, Mar 19, 2020 at 4:10 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> In this PR we had a 512-bit VECTOR_TYPE whose mode is XImode
> (an integer mode used for four 128-bit vectors).  When trying
> to expand a zero constant for it, we hit code in expand_expr_real_1
> that tries to use the associated integer type instead.  The code used
> type_for_mode (XImode, 1) to get this integer type.
>
> However, the c-family implementation of type_for_mode checks for
> any registered built-in type that matches the mode and has the
> right signedness.  This meant that it could return a built-in
> vector type when given an integer mode (particularly if, as here,
> the vector type isn't supported by the current subtarget and so
> TYPE_MODE != TYPE_MODE_RAW).  The expand code would then cycle
> endlessly trying to use this "new" type instead of the original
> vector type.
>
> The search loop is probably too lax in other ways -- e.g. it could
> return records that just happen to have the right mode -- but this
> seems like a safe, incremental improvement.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?
>
> Richard
>
>
> 2020-03-18  Richard Sandiford  <richard.sandiford@arm.com>
>
> gcc/c-family/
>         PR middle-end/94072
>         * c-common.c (c_common_type_for_mode): Before using a registered
>         built-in type, check that the vectorness of the type matches
>         the vectorness of the mode.
>
> gcc/testsuite/
>         PR middle-end/94072
>         * gcc.target/aarch64/pr94072.c: New test.
> ---
>  gcc/c-family/c-common.c                    | 11 +++++++----
>  gcc/testsuite/gcc.target/aarch64/pr94072.c |  9 +++++++++
>  2 files changed, 16 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/pr94072.c
>
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index 25020bf1415..8e5a9243827 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -2387,10 +2387,13 @@ c_common_type_for_mode (machine_mode mode, int unsignedp)
>      }
>
>    for (t = registered_builtin_types; t; t = TREE_CHAIN (t))
> -    if (TYPE_MODE (TREE_VALUE (t)) == mode
> -       && !!unsignedp == !!TYPE_UNSIGNED (TREE_VALUE (t)))
> -      return TREE_VALUE (t);
> -
> +    {
> +      tree type = TREE_VALUE (t);
> +      if (TYPE_MODE (type) == mode
> +         && VECTOR_TYPE_P (type) == VECTOR_MODE_P (mode)
> +         && !!unsignedp == !!TYPE_UNSIGNED (type))
> +       return type;
> +    }
>    return NULL_TREE;
>  }
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr94072.c b/gcc/testsuite/gcc.target/aarch64/pr94072.c
> new file mode 100644
> index 00000000000..2aa72eb7a16
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr94072.c
> @@ -0,0 +1,9 @@
> +/* { dg-options "-msve-vector-bits=512" } */
> +
> +#pragma GCC target "+nosve"
> +
> +void
> +foo (void)
> +{
> +  (int __attribute__ ((__vector_size__ (64)))){};
> +}

Shouldn't this test also be enabled for AVX512F?
Richard Sandiford March 19, 2020, 2 p.m. UTC | #2
"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Thu, Mar 19, 2020 at 4:10 AM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> In this PR we had a 512-bit VECTOR_TYPE whose mode is XImode
>> (an integer mode used for four 128-bit vectors).  When trying
>> to expand a zero constant for it, we hit code in expand_expr_real_1
>> that tries to use the associated integer type instead.  The code used
>> type_for_mode (XImode, 1) to get this integer type.
>>
>> However, the c-family implementation of type_for_mode checks for
>> any registered built-in type that matches the mode and has the
>> right signedness.  This meant that it could return a built-in
>> vector type when given an integer mode (particularly if, as here,
>> the vector type isn't supported by the current subtarget and so
>> TYPE_MODE != TYPE_MODE_RAW).  The expand code would then cycle
>> endlessly trying to use this "new" type instead of the original
>> vector type.
>>
>> The search loop is probably too lax in other ways -- e.g. it could
>> return records that just happen to have the right mode -- but this
>> seems like a safe, incremental improvement.
>>
>> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?
>>
>> Richard
>>
>>
>> 2020-03-18  Richard Sandiford  <richard.sandiford@arm.com>
>>
>> gcc/c-family/
>>         PR middle-end/94072
>>         * c-common.c (c_common_type_for_mode): Before using a registered
>>         built-in type, check that the vectorness of the type matches
>>         the vectorness of the mode.
>>
>> gcc/testsuite/
>>         PR middle-end/94072
>>         * gcc.target/aarch64/pr94072.c: New test.
>> ---
>>  gcc/c-family/c-common.c                    | 11 +++++++----
>>  gcc/testsuite/gcc.target/aarch64/pr94072.c |  9 +++++++++
>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.target/aarch64/pr94072.c
>>
>> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
>> index 25020bf1415..8e5a9243827 100644
>> --- a/gcc/c-family/c-common.c
>> +++ b/gcc/c-family/c-common.c
>> @@ -2387,10 +2387,13 @@ c_common_type_for_mode (machine_mode mode, int unsignedp)
>>      }
>>
>>    for (t = registered_builtin_types; t; t = TREE_CHAIN (t))
>> -    if (TYPE_MODE (TREE_VALUE (t)) == mode
>> -       && !!unsignedp == !!TYPE_UNSIGNED (TREE_VALUE (t)))
>> -      return TREE_VALUE (t);
>> -
>> +    {
>> +      tree type = TREE_VALUE (t);
>> +      if (TYPE_MODE (type) == mode
>> +         && VECTOR_TYPE_P (type) == VECTOR_MODE_P (mode)
>> +         && !!unsignedp == !!TYPE_UNSIGNED (type))
>> +       return type;
>> +    }
>>    return NULL_TREE;
>>  }
>>
>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr94072.c b/gcc/testsuite/gcc.target/aarch64/pr94072.c
>> new file mode 100644
>> index 00000000000..2aa72eb7a16
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/pr94072.c
>> @@ -0,0 +1,9 @@
>> +/* { dg-options "-msve-vector-bits=512" } */
>> +
>> +#pragma GCC target "+nosve"
>> +
>> +void
>> +foo (void)
>> +{
>> +  (int __attribute__ ((__vector_size__ (64)))){};
>> +}
>
> Shouldn't this test also be enabled for AVX512F?

The test already worked on 512-bit vector targets (including 512-bit SVE).
The problem was when the SVE length was set to 512 bits but SVE itself
wasn't enabled.

There are already more extensive tests for:

  int __attribute__ ((__vector_size__ (64)))

and similar vectors in gcc.dg and gcc.c-torture, so I don't think
making this generic would really add much.

Thanks,
Richard
Joseph Myers March 19, 2020, 9:41 p.m. UTC | #3
On Thu, 19 Mar 2020, Richard Sandiford wrote:

> In this PR we had a 512-bit VECTOR_TYPE whose mode is XImode
> (an integer mode used for four 128-bit vectors).  When trying
> to expand a zero constant for it, we hit code in expand_expr_real_1
> that tries to use the associated integer type instead.  The code used
> type_for_mode (XImode, 1) to get this integer type.
> 
> However, the c-family implementation of type_for_mode checks for
> any registered built-in type that matches the mode and has the
> right signedness.  This meant that it could return a built-in
> vector type when given an integer mode (particularly if, as here,
> the vector type isn't supported by the current subtarget and so
> TYPE_MODE != TYPE_MODE_RAW).  The expand code would then cycle
> endlessly trying to use this "new" type instead of the original
> vector type.
> 
> The search loop is probably too lax in other ways -- e.g. it could
> return records that just happen to have the right mode -- but this
> seems like a safe, incremental improvement.
> 
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

OK.
diff mbox series

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 25020bf1415..8e5a9243827 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -2387,10 +2387,13 @@  c_common_type_for_mode (machine_mode mode, int unsignedp)
     }
 
   for (t = registered_builtin_types; t; t = TREE_CHAIN (t))
-    if (TYPE_MODE (TREE_VALUE (t)) == mode
-	&& !!unsignedp == !!TYPE_UNSIGNED (TREE_VALUE (t)))
-      return TREE_VALUE (t);
-
+    {
+      tree type = TREE_VALUE (t);
+      if (TYPE_MODE (type) == mode
+	  && VECTOR_TYPE_P (type) == VECTOR_MODE_P (mode)
+	  && !!unsignedp == !!TYPE_UNSIGNED (type))
+	return type;
+    }
   return NULL_TREE;
 }
 
diff --git a/gcc/testsuite/gcc.target/aarch64/pr94072.c b/gcc/testsuite/gcc.target/aarch64/pr94072.c
new file mode 100644
index 00000000000..2aa72eb7a16
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr94072.c
@@ -0,0 +1,9 @@ 
+/* { dg-options "-msve-vector-bits=512" } */
+
+#pragma GCC target "+nosve"
+
+void
+foo (void)
+{
+  (int __attribute__ ((__vector_size__ (64)))){};
+}