nvptx: Fix ICE in nvptx_vector_alignment on gcc.dg/attr-vector_size.c

Message ID 00ee01d64e39$7c796040$756c20c0$@nextmovesoftware.com
State New
Headers show
Series
  • nvptx: Fix ICE in nvptx_vector_alignment on gcc.dg/attr-vector_size.c
Related show

Commit Message

Roger Sayle June 29, 2020, 5:19 p.m.
This patch addresses the ICE in gcc.dg/attr-vector_size.c during make -k check on
nvptx-none.  The actual ICE looks like:

testsuite/gcc.dg/attr-vector_size.c:29:1: internal compiler error: in tree_to_shwi, at tree.c:7321
0xf53bf2 tree_to_shwi(tree_node const*)
        ../../gcc/gcc/tree.c:7321
0xff1969 nvptx_vector_alignment
        ../../gcc/gcc/config/nvptx/nvptx.c:5105^M

The problem is that the caller has ensured that TYPE_SIZE(type) is representable as
an unsigned HOST_WIDE_INT, but nvptx_vector_alignment is accessing it as a
signed HOST_WIDE_INT which overflows in pathological conditions.  Amongst those
pathological conditions is that a TYPE_SIZE of zero can sometimes reach this function,
prior to an error being emitted.  Making sure the result is not less than the mode's
alignment and not greater than BIGGEST_ALIGNMENT fixes the ICEs, and generates 
the expected compile-time error messages.

Tested on --target=nvptx-none, with a "make" and "make check" which results in four
fewer unexpected failures and three more expected passes.
Ok for mainline?


2020-06-29  Roger Sayle  <roger@nextmovesoftware.com>

gcc/ChangeLog:
        * config/nvptx/nvptx.c (nvptx_vector_alignment): Use tree_to_uhwi
        to access TYPE_SIZE (type).  Return at least the mode's alignment.

Thanks,
Roger
--
Roger Sayle
NextMove Software
Cambridge, UK

Comments

Tom de Vries July 2, 2020, 12:08 p.m. | #1
On 6/29/20 7:19 PM, Roger Sayle wrote:
> This patch addresses the ICE in gcc.dg/attr-vector_size.c during make -k check on

> nvptx-none.  The actual ICE looks like:

> 

> testsuite/gcc.dg/attr-vector_size.c:29:1: internal compiler error: in tree_to_shwi, at tree.c:7321

> 0xf53bf2 tree_to_shwi(tree_node const*)

>         ../../gcc/gcc/tree.c:7321

> 0xff1969 nvptx_vector_alignment

>         ../../gcc/gcc/config/nvptx/nvptx.c:5105^M

> 

> The problem is that the caller has ensured that TYPE_SIZE(type) is representable as

> an unsigned HOST_WIDE_INT, but nvptx_vector_alignment is accessing it as a

> signed HOST_WIDE_INT which overflows in pathological conditions.  Amongst those

> pathological conditions is that a TYPE_SIZE of zero can sometimes reach this function,

> prior to an error being emitted.  Making sure the result is not less than the mode's

> alignment and not greater than BIGGEST_ALIGNMENT fixes the ICEs, and generates 

> the expected compile-time error messages.

> 

> Tested on --target=nvptx-none, with a "make" and "make check" which results in four

> fewer unexpected failures and three more expected passes.

> Ok for mainline?

> 

> 

> 2020-06-29  Roger Sayle  <roger@nextmovesoftware.com>

> 

> gcc/ChangeLog:

>         * config/nvptx/nvptx.c (nvptx_vector_alignment): Use tree_to_uhwi

>         to access TYPE_SIZE (type).  Return at least the mode's alignment.

> 

> Thanks,

> Roger

> --

> Roger Sayle

> NextMove Software

> Cambridge, UK

> 

> diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c

> index e3e84df..bfad91b 100644

> --- a/gcc/config/nvptx/nvptx.c

> +++ b/gcc/config/nvptx/nvptx.c

> @@ -5102,9 +5102,10 @@ static const struct attribute_spec nvptx_attribute_table[] =

>  static HOST_WIDE_INT

>  nvptx_vector_alignment (const_tree type)

>  {

> -  HOST_WIDE_INT align = tree_to_shwi (TYPE_SIZE (type));

> -

> -  return MIN (align, BIGGEST_ALIGNMENT);

> +  unsigned HOST_WIDE_INT align = tree_to_uhwi (TYPE_SIZE (type));


In the default target hook, we test tree_fits_uhwi_p, so I prefer we do
the same.

> +  if (align > BIGGEST_ALIGNMENT)

> +    return BIGGEST_ALIGNMENT;


I prefer using MIN to make code easier to read.

> +  return MAX (align, GET_MODE_ALIGNMENT (TYPE_MODE (type)));

>  }

>  

>  /* Indicate that INSN cannot be duplicated.   */


Also, this is related to a PR, number included in commit log.

As of yet untested updated patch attached.

Thanks,
- Tom
nvptx: Fix ICE in nvptx_vector_alignment on gcc.dg/attr-vector_size.c

This patch addresses the ICE in gcc.dg/attr-vector_size.c during
make -k check on nvptx-none.  The actual ICE looks like:

testsuite/gcc.dg/attr-vector_size.c:29:1: internal compiler error: \
  in tree_to_shwi, at tree.c:7321
0xf53bf2 tree_to_shwi(tree_node const*)
../../gcc/gcc/tree.c:7321
0xff1969 nvptx_vector_alignment
../../gcc/gcc/config/nvptx/nvptx.c:5105^M

The problem is that the caller has ensured that TYPE_SIZE(type) is
representable as an unsigned HOST_WIDE_INT, but nvptx_vector_alignment is
accessing it as a signed HOST_WIDE_INT which overflows in pathological
conditions.  Amongst those pathological conditions is that a TYPE_SIZE of
zero can sometimes reach this function, prior to an error being emitted.
Making sure the result is not less than the mode's alignment and not greater
than BIGGEST_ALIGNMENT fixes the ICEs, and generates the expected
compile-time error messages.

Tested on --target=nvptx-none, with a "make" and "make check" which results
in four fewer unexpected failures and three more expected passes.

2020-06-29  Roger Sayle  <roger@nextmovesoftware.com>

gcc/ChangeLog:

	PR target/90932
	* config/nvptx/nvptx.c (nvptx_vector_alignment): Use tree_to_uhwi
	to access TYPE_SIZE (type).  Return at least the mode's alignment.

---
 gcc/config/nvptx/nvptx.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c
index e3e84dfd4e4..d2f321fcbcc 100644
--- a/gcc/config/nvptx/nvptx.c
+++ b/gcc/config/nvptx/nvptx.c
@@ -5102,9 +5102,22 @@ static const struct attribute_spec nvptx_attribute_table[] =
 static HOST_WIDE_INT
 nvptx_vector_alignment (const_tree type)
 {
-  HOST_WIDE_INT align = tree_to_shwi (TYPE_SIZE (type));
+  unsigned HOST_WIDE_INT align;
+  tree size = TYPE_SIZE (type);
+
+  /* Ensure align is not bigger than BIGGEST_ALIGNMENT.  */
+  if (tree_fits_uhwi_p (size))
+    {
+      align = tree_to_uhwi (size);
+      align = MIN (align, BIGGEST_ALIGNMENT);
+    }
+  else
+    align = BIGGEST_ALIGNMENT;
+
+  /* Ensure align is not smaller than mode alignment.  */
+  align = MAX (align, GET_MODE_ALIGNMENT (TYPE_MODE (type)));
 
-  return MIN (align, BIGGEST_ALIGNMENT);
+  return align;
 }
 
 /* Indicate that INSN cannot be duplicated.   */
Roger Sayle July 2, 2020, 1:10 p.m. | #2
Hi Tom,

Beware of doing something just because the default target hook does it that way.
See https://gcc.gnu.org/pipermail/gcc-patches/2020-June/549128.html
which fixes PR middle-end/90597 by correcting default_vector_alignment to do
it the same way as proposed by this nvptx backend patch.

Many thanks for pointing out that the nvptx problem is PR target/90932.

Roger
--

-----Original Message-----
From: Tom de Vries <tdevries@suse.de> 

Sent: 02 July 2020 13:09
To: Roger Sayle <roger@nextmovesoftware.com>; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] nvptx: Fix ICE in nvptx_vector_alignment on gcc.dg/attr-vector_size.c

On 6/29/20 7:19 PM, Roger Sayle wrote:
> This patch addresses the ICE in gcc.dg/attr-vector_size.c during make 

> -k check on nvptx-none.  The actual ICE looks like:

> 

> testsuite/gcc.dg/attr-vector_size.c:29:1: internal compiler error: in 

> tree_to_shwi, at tree.c:7321

> 0xf53bf2 tree_to_shwi(tree_node const*)

>         ../../gcc/gcc/tree.c:7321

> 0xff1969 nvptx_vector_alignment

>         ../../gcc/gcc/config/nvptx/nvptx.c:5105^M

> 

> The problem is that the caller has ensured that TYPE_SIZE(type) is 

> representable as an unsigned HOST_WIDE_INT, but nvptx_vector_alignment 

> is accessing it as a signed HOST_WIDE_INT which overflows in 

> pathological conditions.  Amongst those pathological conditions is 

> that a TYPE_SIZE of zero can sometimes reach this function, prior to 

> an error being emitted.  Making sure the result is not less than the 

> mode's alignment and not greater than BIGGEST_ALIGNMENT fixes the ICEs, and generates the expected compile-time error messages.

> 

> Tested on --target=nvptx-none, with a "make" and "make check" which 

> results in four fewer unexpected failures and three more expected passes.

> Ok for mainline?

> 

> 

> 2020-06-29  Roger Sayle  <roger@nextmovesoftware.com>

> 

> gcc/ChangeLog:

>         * config/nvptx/nvptx.c (nvptx_vector_alignment): Use tree_to_uhwi

>         to access TYPE_SIZE (type).  Return at least the mode's alignment.

> 

> Thanks,

> Roger

> --

> Roger Sayle

> NextMove Software

> Cambridge, UK

> 

> diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c index 

> e3e84df..bfad91b 100644

> --- a/gcc/config/nvptx/nvptx.c

> +++ b/gcc/config/nvptx/nvptx.c

> @@ -5102,9 +5102,10 @@ static const struct attribute_spec 

> nvptx_attribute_table[] =  static HOST_WIDE_INT  

> nvptx_vector_alignment (const_tree type)  {

> -  HOST_WIDE_INT align = tree_to_shwi (TYPE_SIZE (type));

> -

> -  return MIN (align, BIGGEST_ALIGNMENT);

> +  unsigned HOST_WIDE_INT align = tree_to_uhwi (TYPE_SIZE (type));


In the default target hook, we test tree_fits_uhwi_p, so I prefer we do the same.

> +  if (align > BIGGEST_ALIGNMENT)

> +    return BIGGEST_ALIGNMENT;


I prefer using MIN to make code easier to read.

> +  return MAX (align, GET_MODE_ALIGNMENT (TYPE_MODE (type)));

>  }

>  

>  /* Indicate that INSN cannot be duplicated.   */


Also, this is related to a PR, number included in commit log.

As of yet untested updated patch attached.

Thanks,
- Tom
Tom de Vries July 2, 2020, 5:23 p.m. | #3
On 7/2/20 3:10 PM, Roger Sayle wrote:
> Hi Tom,

> 

> Beware of doing something just because the default target hook does it that way.


Well, what I'm saying is that if the default target hook doesn't assume
tree_fits_uhwi_p (size), the safest solution is to do the same in the
nvptx target hook.

Thanks,
- Tom

> See https://gcc.gnu.org/pipermail/gcc-patches/2020-June/549128.html

> which fixes PR middle-end/90597 by correcting default_vector_alignment to do

> it the same way as proposed by this nvptx backend patch.

> 

> Many thanks for pointing out that the nvptx problem is PR target/90932.

> 

> Roger

> --

> 

> -----Original Message-----

> From: Tom de Vries <tdevries@suse.de> 

> Sent: 02 July 2020 13:09

> To: Roger Sayle <roger@nextmovesoftware.com>; gcc-patches@gcc.gnu.org

> Subject: Re: [PATCH] nvptx: Fix ICE in nvptx_vector_alignment on gcc.dg/attr-vector_size.c

> 

> On 6/29/20 7:19 PM, Roger Sayle wrote:

>> This patch addresses the ICE in gcc.dg/attr-vector_size.c during make 

>> -k check on nvptx-none.  The actual ICE looks like:

>>

>> testsuite/gcc.dg/attr-vector_size.c:29:1: internal compiler error: in 

>> tree_to_shwi, at tree.c:7321

>> 0xf53bf2 tree_to_shwi(tree_node const*)

>>         ../../gcc/gcc/tree.c:7321

>> 0xff1969 nvptx_vector_alignment

>>         ../../gcc/gcc/config/nvptx/nvptx.c:5105^M

>>

>> The problem is that the caller has ensured that TYPE_SIZE(type) is 

>> representable as an unsigned HOST_WIDE_INT, but nvptx_vector_alignment 

>> is accessing it as a signed HOST_WIDE_INT which overflows in 

>> pathological conditions.  Amongst those pathological conditions is 

>> that a TYPE_SIZE of zero can sometimes reach this function, prior to 

>> an error being emitted.  Making sure the result is not less than the 

>> mode's alignment and not greater than BIGGEST_ALIGNMENT fixes the ICEs, and generates the expected compile-time error messages.

>>

>> Tested on --target=nvptx-none, with a "make" and "make check" which 

>> results in four fewer unexpected failures and three more expected passes.

>> Ok for mainline?

>>

>>

>> 2020-06-29  Roger Sayle  <roger@nextmovesoftware.com>

>>

>> gcc/ChangeLog:

>>         * config/nvptx/nvptx.c (nvptx_vector_alignment): Use tree_to_uhwi

>>         to access TYPE_SIZE (type).  Return at least the mode's alignment.

>>

>> Thanks,

>> Roger

>> --

>> Roger Sayle

>> NextMove Software

>> Cambridge, UK

>>

>> diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c index 

>> e3e84df..bfad91b 100644

>> --- a/gcc/config/nvptx/nvptx.c

>> +++ b/gcc/config/nvptx/nvptx.c

>> @@ -5102,9 +5102,10 @@ static const struct attribute_spec 

>> nvptx_attribute_table[] =  static HOST_WIDE_INT  

>> nvptx_vector_alignment (const_tree type)  {

>> -  HOST_WIDE_INT align = tree_to_shwi (TYPE_SIZE (type));

>> -

>> -  return MIN (align, BIGGEST_ALIGNMENT);

>> +  unsigned HOST_WIDE_INT align = tree_to_uhwi (TYPE_SIZE (type));

> 

> In the default target hook, we test tree_fits_uhwi_p, so I prefer we do the same.

> 

>> +  if (align > BIGGEST_ALIGNMENT)

>> +    return BIGGEST_ALIGNMENT;

> 

> I prefer using MIN to make code easier to read.

> 

>> +  return MAX (align, GET_MODE_ALIGNMENT (TYPE_MODE (type)));

>>  }

>>  

>>  /* Indicate that INSN cannot be duplicated.   */

> 

> Also, this is related to a PR, number included in commit log.

> 

> As of yet untested updated patch attached.

> 

> Thanks,

> - Tom

>

Patch

diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c
index e3e84df..bfad91b 100644
--- a/gcc/config/nvptx/nvptx.c
+++ b/gcc/config/nvptx/nvptx.c
@@ -5102,9 +5102,10 @@  static const struct attribute_spec nvptx_attribute_table[] =
 static HOST_WIDE_INT
 nvptx_vector_alignment (const_tree type)
 {
-  HOST_WIDE_INT align = tree_to_shwi (TYPE_SIZE (type));
-
-  return MIN (align, BIGGEST_ALIGNMENT);
+  unsigned HOST_WIDE_INT align = tree_to_uhwi (TYPE_SIZE (type));
+  if (align > BIGGEST_ALIGNMENT)
+    return BIGGEST_ALIGNMENT;
+  return MAX (align, GET_MODE_ALIGNMENT (TYPE_MODE (type)));
 }
 
 /* Indicate that INSN cannot be duplicated.   */