| Message ID | mpta74c4z2t.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | c-family: Tighten vector handling in type_for_mode [PR94072] | expand |
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?
"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
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 --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)))){}; +}