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 | expand |
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
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
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 >
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. */