| Message ID | patch-18679-tamar@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | AArch64: check for vector mode in get_mask_mode [PR116074] | expand |
Tamar Christina <tamar.christina@arm.com> writes: > 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. Feels to me like the problem is higher up. The hook says: Return the mode to use for a vector mask that holds one boolean result for each element of vector mode @var{mode}. ... So the caller is breaking the interface if it is passing a non-vector mode. Thanks, Richard > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > PR target/116074 > * config/aarch64/aarch64.cc (aarch64_get_mask_mode): Check vector mode. > > gcc/testsuite/ChangeLog: > > PR target/116074 > * g++.target/aarch64/pr116074.C: New test. > > --- > > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index 355ab97891cf0a7d487fa4c69ae23a5f75897851..045ac0e09b0eaa14935db3924798402c9dd1947c 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -1870,6 +1870,9 @@ aarch64_sve_pred_mode (machine_mode mode) > static opt_machine_mode > aarch64_get_mask_mode (machine_mode mode) > { > + if (!VECTOR_MODE_P (mode)) > + return opt_machine_mode (); > + > unsigned int vec_flags = aarch64_classify_vector_mode (mode); > if (vec_flags & VEC_SVE_DATA) > return aarch64_sve_pred_mode (mode); > 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; > + } > +}
> -----Original Message----- > From: Richard Sandiford <richard.sandiford@arm.com> > Sent: Friday, July 26, 2024 10:24 AM > To: Tamar Christina <Tamar.Christina@arm.com> > Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw > <Richard.Earnshaw@arm.com>; Marcus Shawcroft > <Marcus.Shawcroft@arm.com>; ktkachov@gcc.gnu.org > Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode > [PR116074] > > Tamar Christina <tamar.christina@arm.com> writes: > > 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. > > Feels to me like the problem is higher up. The hook says: > > Return the mode to use for a vector mask that holds one boolean > result for each element of vector mode @var{mode}. ... > > So the caller is breaking the interface if it is passing a non-vector mode. But my point is that it's AArch64 that creates a vector type from a non-vector mode. I can easily fix my own call site. But it's weird that AArch64 has a TYPE_MODE (vectorype) != VECTOR_MODE. So I still think this is an AArch64 bug. Tamar > > Thanks, > Richard > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > > > Ok for master? > > > > Thanks, > > Tamar > > > > gcc/ChangeLog: > > > > PR target/116074 > > * config/aarch64/aarch64.cc (aarch64_get_mask_mode): Check vector > mode. > > > > gcc/testsuite/ChangeLog: > > > > PR target/116074 > > * g++.target/aarch64/pr116074.C: New test. > > > > --- > > > > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > > index > 355ab97891cf0a7d487fa4c69ae23a5f75897851..045ac0e09b0eaa14935db392 > 4798402c9dd1947c 100644 > > --- a/gcc/config/aarch64/aarch64.cc > > +++ b/gcc/config/aarch64/aarch64.cc > > @@ -1870,6 +1870,9 @@ aarch64_sve_pred_mode (machine_mode mode) > > static opt_machine_mode > > aarch64_get_mask_mode (machine_mode mode) > > { > > + if (!VECTOR_MODE_P (mode)) > > + return opt_machine_mode (); > > + > > unsigned int vec_flags = aarch64_classify_vector_mode (mode); > > if (vec_flags & VEC_SVE_DATA) > > return aarch64_sve_pred_mode (mode); > > diff --git a/gcc/testsuite/g++.target/aarch64/pr116074.C > b/gcc/testsuite/g++.target/aarch64/pr116074.C > > new file mode 100644 > > index > 0000000000000000000000000000000000000000..54cf561510c460499a816a > b6a84603fc20a5f1e5 > > --- /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; > > + } > > +}
> Am 26.07.2024 um 11:29 schrieb Tamar Christina <tamar.christina@arm.com>: > > >> >> -----Original Message----- >> From: Richard Sandiford <richard.sandiford@arm.com> >> Sent: Friday, July 26, 2024 10:24 AM >> To: Tamar Christina <Tamar.Christina@arm.com> >> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw >> <Richard.Earnshaw@arm.com>; Marcus Shawcroft >> <Marcus.Shawcroft@arm.com>; ktkachov@gcc.gnu.org >> Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode >> [PR116074] >> >> Tamar Christina <tamar.christina@arm.com> writes: >>> 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. >> >> Feels to me like the problem is higher up. The hook says: >> >> Return the mode to use for a vector mask that holds one boolean >> result for each element of vector mode @var{mode}. ... >> >> So the caller is breaking the interface if it is passing a non-vector mode. > > But my point is that it's AArch64 that creates a vector type from a non-vector mode. > I can easily fix my own call site. But it's weird that AArch64 has a TYPE_MODE (vectorype) != VECTOR_MODE. > > So I still think this is an AArch64 bug. Note the middle-end creates such vector types as well, though usually not with TImode. Generally I’d consider using the Scalar mode for vector(1) appropriate. Richard > Tamar >> >> Thanks, >> Richard >> >>> >>> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. >>> >>> Ok for master? >>> >>> Thanks, >>> Tamar >>> >>> gcc/ChangeLog: >>> >>> PR target/116074 >>> * config/aarch64/aarch64.cc (aarch64_get_mask_mode): Check vector >> mode. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR target/116074 >>> * g++.target/aarch64/pr116074.C: New test. >>> >>> --- >>> >>> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc >>> index >> 355ab97891cf0a7d487fa4c69ae23a5f75897851..045ac0e09b0eaa14935db392 >> 4798402c9dd1947c 100644 >>> --- a/gcc/config/aarch64/aarch64.cc >>> +++ b/gcc/config/aarch64/aarch64.cc >>> @@ -1870,6 +1870,9 @@ aarch64_sve_pred_mode (machine_mode mode) >>> static opt_machine_mode >>> aarch64_get_mask_mode (machine_mode mode) >>> { >>> + if (!VECTOR_MODE_P (mode)) >>> + return opt_machine_mode (); >>> + >>> unsigned int vec_flags = aarch64_classify_vector_mode (mode); >>> if (vec_flags & VEC_SVE_DATA) >>> return aarch64_sve_pred_mode (mode); >>> diff --git a/gcc/testsuite/g++.target/aarch64/pr116074.C >> b/gcc/testsuite/g++.target/aarch64/pr116074.C >>> new file mode 100644 >>> index >> 0000000000000000000000000000000000000000..54cf561510c460499a816a >> b6a84603fc20a5f1e5 >>> --- /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; >>> + } >>> +}
Tamar Christina <Tamar.Christina@arm.com> writes: >> -----Original Message----- >> From: Richard Sandiford <richard.sandiford@arm.com> >> Sent: Friday, July 26, 2024 10:24 AM >> To: Tamar Christina <Tamar.Christina@arm.com> >> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw >> <Richard.Earnshaw@arm.com>; Marcus Shawcroft >> <Marcus.Shawcroft@arm.com>; ktkachov@gcc.gnu.org >> Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode >> [PR116074] >> >> Tamar Christina <tamar.christina@arm.com> writes: >> > 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. >> >> Feels to me like the problem is higher up. The hook says: >> >> Return the mode to use for a vector mask that holds one boolean >> result for each element of vector mode @var{mode}. ... >> >> So the caller is breaking the interface if it is passing a non-vector mode. > > But my point is that it's AArch64 that creates a vector type from a non-vector mode. > I can easily fix my own call site. But it's weird that AArch64 has a TYPE_MODE (vectorype) != VECTOR_MODE. Vector types without vector modes aren't unusual in themselves. It's how many "simple" vector types are represented on non-vector ISAs. And on aarch64: typedef unsigned char foo __attribute__((vector_size(2))); is a vector type with HImode. I don't think the hook is expected to deal with those. The vectoriser also directly supports vector types with integer modes in limited cases, via vect_can_vectorize_without_simd_p. Thanks, Richard > > So I still think this is an AArch64 bug. > > Tamar >> >> Thanks, >> Richard >> >> > >> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. >> > >> > Ok for master? >> > >> > Thanks, >> > Tamar >> > >> > gcc/ChangeLog: >> > >> > PR target/116074 >> > * config/aarch64/aarch64.cc (aarch64_get_mask_mode): Check vector >> mode. >> > >> > gcc/testsuite/ChangeLog: >> > >> > PR target/116074 >> > * g++.target/aarch64/pr116074.C: New test. >> > >> > --- >> > >> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc >> > index >> 355ab97891cf0a7d487fa4c69ae23a5f75897851..045ac0e09b0eaa14935db392 >> 4798402c9dd1947c 100644 >> > --- a/gcc/config/aarch64/aarch64.cc >> > +++ b/gcc/config/aarch64/aarch64.cc >> > @@ -1870,6 +1870,9 @@ aarch64_sve_pred_mode (machine_mode mode) >> > static opt_machine_mode >> > aarch64_get_mask_mode (machine_mode mode) >> > { >> > + if (!VECTOR_MODE_P (mode)) >> > + return opt_machine_mode (); >> > + >> > unsigned int vec_flags = aarch64_classify_vector_mode (mode); >> > if (vec_flags & VEC_SVE_DATA) >> > return aarch64_sve_pred_mode (mode); >> > diff --git a/gcc/testsuite/g++.target/aarch64/pr116074.C >> b/gcc/testsuite/g++.target/aarch64/pr116074.C >> > new file mode 100644 >> > index >> 0000000000000000000000000000000000000000..54cf561510c460499a816a >> b6a84603fc20a5f1e5 >> > --- /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; >> > + } >> > +}
> -----Original Message----- > From: Richard Sandiford <richard.sandiford@arm.com> > Sent: Friday, July 26, 2024 10:43 AM > To: Tamar Christina <Tamar.Christina@arm.com> > Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw > <Richard.Earnshaw@arm.com>; Marcus Shawcroft > <Marcus.Shawcroft@arm.com>; ktkachov@gcc.gnu.org > Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode > [PR116074] > > Tamar Christina <Tamar.Christina@arm.com> writes: > >> -----Original Message----- > >> From: Richard Sandiford <richard.sandiford@arm.com> > >> Sent: Friday, July 26, 2024 10:24 AM > >> To: Tamar Christina <Tamar.Christina@arm.com> > >> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw > >> <Richard.Earnshaw@arm.com>; Marcus Shawcroft > >> <Marcus.Shawcroft@arm.com>; ktkachov@gcc.gnu.org > >> Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode > >> [PR116074] > >> > >> Tamar Christina <tamar.christina@arm.com> writes: > >> > 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. > >> > >> Feels to me like the problem is higher up. The hook says: > >> > >> Return the mode to use for a vector mask that holds one boolean > >> result for each element of vector mode @var{mode}. ... > >> > >> So the caller is breaking the interface if it is passing a non-vector mode. > > > > But my point is that it's AArch64 that creates a vector type from a non-vector > mode. > > I can easily fix my own call site. But it's weird that AArch64 has a TYPE_MODE > (vectorype) != VECTOR_MODE. > > Vector types without vector modes aren't unusual in themselves. > It's how many "simple" vector types are represented on non-vector ISAs. > And on aarch64: > > typedef unsigned char foo __attribute__((vector_size(2))); > > is a vector type with HImode. I don't think the hook is expected to > deal with those. My point is that even in this scenario it would be better for the hook to return False. I don't understand why in this case it's better to ICE. Clearly there's no vector mask for this mode, and this now complicates every call site. Put it differently, what's the point in making the usage of the hook harder for no apparent gain. Tamar > > The vectoriser also directly supports vector types with integer modes > in limited cases, via vect_can_vectorize_without_simd_p. > > Thanks, > Richard > > > > So I still think this is an AArch64 bug. > > > > Tamar > >> > >> Thanks, > >> Richard > >> > >> > > >> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > >> > > >> > Ok for master? > >> > > >> > Thanks, > >> > Tamar > >> > > >> > gcc/ChangeLog: > >> > > >> > PR target/116074 > >> > * config/aarch64/aarch64.cc (aarch64_get_mask_mode): Check vector > >> mode. > >> > > >> > gcc/testsuite/ChangeLog: > >> > > >> > PR target/116074 > >> > * g++.target/aarch64/pr116074.C: New test. > >> > > >> > --- > >> > > >> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > >> > index > >> > 355ab97891cf0a7d487fa4c69ae23a5f75897851..045ac0e09b0eaa14935db392 > >> 4798402c9dd1947c 100644 > >> > --- a/gcc/config/aarch64/aarch64.cc > >> > +++ b/gcc/config/aarch64/aarch64.cc > >> > @@ -1870,6 +1870,9 @@ aarch64_sve_pred_mode (machine_mode mode) > >> > static opt_machine_mode > >> > aarch64_get_mask_mode (machine_mode mode) > >> > { > >> > + if (!VECTOR_MODE_P (mode)) > >> > + return opt_machine_mode (); > >> > + > >> > unsigned int vec_flags = aarch64_classify_vector_mode (mode); > >> > if (vec_flags & VEC_SVE_DATA) > >> > return aarch64_sve_pred_mode (mode); > >> > diff --git a/gcc/testsuite/g++.target/aarch64/pr116074.C > >> b/gcc/testsuite/g++.target/aarch64/pr116074.C > >> > new file mode 100644 > >> > index > >> > 0000000000000000000000000000000000000000..54cf561510c460499a816a > >> b6a84603fc20a5f1e5 > >> > --- /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; > >> > + } > >> > +}
Tamar Christina <Tamar.Christina@arm.com> writes: >> -----Original Message----- >> From: Richard Sandiford <richard.sandiford@arm.com> >> Sent: Friday, July 26, 2024 10:43 AM >> To: Tamar Christina <Tamar.Christina@arm.com> >> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw >> <Richard.Earnshaw@arm.com>; Marcus Shawcroft >> <Marcus.Shawcroft@arm.com>; ktkachov@gcc.gnu.org >> Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode >> [PR116074] >> >> Tamar Christina <Tamar.Christina@arm.com> writes: >> >> -----Original Message----- >> >> From: Richard Sandiford <richard.sandiford@arm.com> >> >> Sent: Friday, July 26, 2024 10:24 AM >> >> To: Tamar Christina <Tamar.Christina@arm.com> >> >> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw >> >> <Richard.Earnshaw@arm.com>; Marcus Shawcroft >> >> <Marcus.Shawcroft@arm.com>; ktkachov@gcc.gnu.org >> >> Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode >> >> [PR116074] >> >> >> >> Tamar Christina <tamar.christina@arm.com> writes: >> >> > 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. >> >> >> >> Feels to me like the problem is higher up. The hook says: >> >> >> >> Return the mode to use for a vector mask that holds one boolean >> >> result for each element of vector mode @var{mode}. ... >> >> >> >> So the caller is breaking the interface if it is passing a non-vector mode. >> > >> > But my point is that it's AArch64 that creates a vector type from a non-vector >> mode. >> > I can easily fix my own call site. But it's weird that AArch64 has a TYPE_MODE >> (vectorype) != VECTOR_MODE. >> >> Vector types without vector modes aren't unusual in themselves. >> It's how many "simple" vector types are represented on non-vector ISAs. >> And on aarch64: >> >> typedef unsigned char foo __attribute__((vector_size(2))); >> >> is a vector type with HImode. I don't think the hook is expected to >> deal with those. > > My point is that even in this scenario it would be better for the hook to return > False. I don't understand why in this case it's better to ICE. > > Clearly there's no vector mask for this mode, and this now complicates every > call site. Put it differently, what's the point in making the usage of the hook > harder for no apparent gain. I guess we'll have to agree to disagree, but your patch for PR116074 seems like the right fix to me. That code wants to know whether the target supports a particular vector operation on a particular vector mode. It should first be sure that it really is dealing with a vector mode. It looks like the RVV hook would also fall over if passed something other than native vector modes. x86 wouldn't, but would try to return a real mode rather than "no mode". This could cause confusion later. E.g. for the x86 equivalent of the 2-char example above, if we have a HImode "vector" mode and the hook returned a HImode mask, we might then go on to ask "ok, can we do this operation on HIs"? And for some optabs (that are defined for both scalars and vectors), the answer might be yes, but the optab would operate on a single HI rather than a pair of QIs. So IMO, it's better to check directly for vector modes, rather than hope that non-vector modes will be rejected as a side effect further down the line. Thanks, Richard > > Tamar > >> >> The vectoriser also directly supports vector types with integer modes >> in limited cases, via vect_can_vectorize_without_simd_p. >> >> Thanks, >> Richard >> > >> > So I still think this is an AArch64 bug. >> > >> > Tamar >> >> >> >> Thanks, >> >> Richard >> >> >> >> > >> >> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. >> >> > >> >> > Ok for master? >> >> > >> >> > Thanks, >> >> > Tamar >> >> > >> >> > gcc/ChangeLog: >> >> > >> >> > PR target/116074 >> >> > * config/aarch64/aarch64.cc (aarch64_get_mask_mode): Check vector >> >> mode. >> >> > >> >> > gcc/testsuite/ChangeLog: >> >> > >> >> > PR target/116074 >> >> > * g++.target/aarch64/pr116074.C: New test. >> >> > >> >> > --- >> >> > >> >> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc >> >> > index >> >> >> 355ab97891cf0a7d487fa4c69ae23a5f75897851..045ac0e09b0eaa14935db392 >> >> 4798402c9dd1947c 100644 >> >> > --- a/gcc/config/aarch64/aarch64.cc >> >> > +++ b/gcc/config/aarch64/aarch64.cc >> >> > @@ -1870,6 +1870,9 @@ aarch64_sve_pred_mode (machine_mode mode) >> >> > static opt_machine_mode >> >> > aarch64_get_mask_mode (machine_mode mode) >> >> > { >> >> > + if (!VECTOR_MODE_P (mode)) >> >> > + return opt_machine_mode (); >> >> > + >> >> > unsigned int vec_flags = aarch64_classify_vector_mode (mode); >> >> > if (vec_flags & VEC_SVE_DATA) >> >> > return aarch64_sve_pred_mode (mode); >> >> > diff --git a/gcc/testsuite/g++.target/aarch64/pr116074.C >> >> b/gcc/testsuite/g++.target/aarch64/pr116074.C >> >> > new file mode 100644 >> >> > index >> >> >> 0000000000000000000000000000000000000000..54cf561510c460499a816a >> >> b6a84603fc20a5f1e5 >> >> > --- /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; >> >> > + } >> >> > +}
On Fri, Jul 26, 2024 at 1:15 PM Richard Sandiford <richard.sandiford@arm.com> wrote: > > Tamar Christina <Tamar.Christina@arm.com> writes: > >> -----Original Message----- > >> From: Richard Sandiford <richard.sandiford@arm.com> > >> Sent: Friday, July 26, 2024 10:43 AM > >> To: Tamar Christina <Tamar.Christina@arm.com> > >> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw > >> <Richard.Earnshaw@arm.com>; Marcus Shawcroft > >> <Marcus.Shawcroft@arm.com>; ktkachov@gcc.gnu.org > >> Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode > >> [PR116074] > >> > >> Tamar Christina <Tamar.Christina@arm.com> writes: > >> >> -----Original Message----- > >> >> From: Richard Sandiford <richard.sandiford@arm.com> > >> >> Sent: Friday, July 26, 2024 10:24 AM > >> >> To: Tamar Christina <Tamar.Christina@arm.com> > >> >> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw > >> >> <Richard.Earnshaw@arm.com>; Marcus Shawcroft > >> >> <Marcus.Shawcroft@arm.com>; ktkachov@gcc.gnu.org > >> >> Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode > >> >> [PR116074] > >> >> > >> >> Tamar Christina <tamar.christina@arm.com> writes: > >> >> > 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. > >> >> > >> >> Feels to me like the problem is higher up. The hook says: > >> >> > >> >> Return the mode to use for a vector mask that holds one boolean > >> >> result for each element of vector mode @var{mode}. ... > >> >> > >> >> So the caller is breaking the interface if it is passing a non-vector mode. > >> > > >> > But my point is that it's AArch64 that creates a vector type from a non-vector > >> mode. > >> > I can easily fix my own call site. But it's weird that AArch64 has a TYPE_MODE > >> (vectorype) != VECTOR_MODE. > >> > >> Vector types without vector modes aren't unusual in themselves. > >> It's how many "simple" vector types are represented on non-vector ISAs. > >> And on aarch64: > >> > >> typedef unsigned char foo __attribute__((vector_size(2))); > >> > >> is a vector type with HImode. I don't think the hook is expected to > >> deal with those. > > > > My point is that even in this scenario it would be better for the hook to return > > False. I don't understand why in this case it's better to ICE. > > > > Clearly there's no vector mask for this mode, and this now complicates every > > call site. Put it differently, what's the point in making the usage of the hook > > harder for no apparent gain. > > I guess we'll have to agree to disagree, but your patch for PR116074 > seems like the right fix to me. That code wants to know whether the > target supports a particular vector operation on a particular vector mode. > It should first be sure that it really is dealing with a vector mode. > > It looks like the RVV hook would also fall over if passed something other > than native vector modes. x86 wouldn't, but would try to return a real > mode rather than "no mode". This could cause confusion later. E.g. > for the x86 equivalent of the 2-char example above, if we have a HImode > "vector" mode and the hook returned a HImode mask, we might then go on > to ask "ok, can we do this operation on HIs"? And for some optabs (that > are defined for both scalars and vectors), the answer might be yes, > but the optab would operate on a single HI rather than a pair of QIs. > > So IMO, it's better to check directly for vector modes, rather than hope > that non-vector modes will be rejected as a side effect further down the line. default_get_mask_mode asserts that it deals with a vector mode (in the called related_int_vector_mode). Richard. > Thanks, > Richard > > > > > > Tamar > > > >> > >> The vectoriser also directly supports vector types with integer modes > >> in limited cases, via vect_can_vectorize_without_simd_p. > >> > >> Thanks, > >> Richard > >> > > >> > So I still think this is an AArch64 bug. > >> > > >> > Tamar > >> >> > >> >> Thanks, > >> >> Richard > >> >> > >> >> > > >> >> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > >> >> > > >> >> > Ok for master? > >> >> > > >> >> > Thanks, > >> >> > Tamar > >> >> > > >> >> > gcc/ChangeLog: > >> >> > > >> >> > PR target/116074 > >> >> > * config/aarch64/aarch64.cc (aarch64_get_mask_mode): Check vector > >> >> mode. > >> >> > > >> >> > gcc/testsuite/ChangeLog: > >> >> > > >> >> > PR target/116074 > >> >> > * g++.target/aarch64/pr116074.C: New test. > >> >> > > >> >> > --- > >> >> > > >> >> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > >> >> > index > >> >> > >> 355ab97891cf0a7d487fa4c69ae23a5f75897851..045ac0e09b0eaa14935db392 > >> >> 4798402c9dd1947c 100644 > >> >> > --- a/gcc/config/aarch64/aarch64.cc > >> >> > +++ b/gcc/config/aarch64/aarch64.cc > >> >> > @@ -1870,6 +1870,9 @@ aarch64_sve_pred_mode (machine_mode mode) > >> >> > static opt_machine_mode > >> >> > aarch64_get_mask_mode (machine_mode mode) > >> >> > { > >> >> > + if (!VECTOR_MODE_P (mode)) > >> >> > + return opt_machine_mode (); > >> >> > + > >> >> > unsigned int vec_flags = aarch64_classify_vector_mode (mode); > >> >> > if (vec_flags & VEC_SVE_DATA) > >> >> > return aarch64_sve_pred_mode (mode); > >> >> > diff --git a/gcc/testsuite/g++.target/aarch64/pr116074.C > >> >> b/gcc/testsuite/g++.target/aarch64/pr116074.C > >> >> > new file mode 100644 > >> >> > index > >> >> > >> 0000000000000000000000000000000000000000..54cf561510c460499a816a > >> >> b6a84603fc20a5f1e5 > >> >> > --- /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/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index 355ab97891cf0a7d487fa4c69ae23a5f75897851..045ac0e09b0eaa14935db3924798402c9dd1947c 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -1870,6 +1870,9 @@ aarch64_sve_pred_mode (machine_mode mode) static opt_machine_mode aarch64_get_mask_mode (machine_mode mode) { + if (!VECTOR_MODE_P (mode)) + return opt_machine_mode (); + unsigned int vec_flags = aarch64_classify_vector_mode (mode); if (vec_flags & VEC_SVE_DATA) return aarch64_sve_pred_mode (mode); 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; + } +}