> -----Original Message-----
> From: Richard Sandiford <richard.sandif...@arm.com>
> Sent: Friday, July 26, 2024 10:43 AM
> To: Tamar Christina <tamar.christ...@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; Richard Earnshaw
> <richard.earns...@arm.com>; Marcus Shawcroft
> <marcus.shawcr...@arm.com>; ktkac...@gcc.gnu.org
> Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode
> [PR116074]
>
> Tamar Christina <tamar.christ...@arm.com> writes:
> >> -----Original Message-----
> >> From: Richard Sandiford <richard.sandif...@arm.com>
> >> Sent: Friday, July 26, 2024 10:24 AM
> >> To: Tamar Christina <tamar.christ...@arm.com>
> >> Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; Richard Earnshaw
> >> <richard.earns...@arm.com>; Marcus Shawcroft
> >> <marcus.shawcr...@arm.com>; ktkac...@gcc.gnu.org
> >> Subject: Re: [PATCH]AArch64: check for vector mode in get_mask_mode
> >> [PR116074]
> >>
> >> Tamar Christina <tamar.christ...@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;
> >> > + }
> >> > +}
