On Fri, Jul 26, 2024 at 1:15 PM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Tamar Christina <tamar.christ...@arm.com> writes:
> >> -----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.
>
> 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;
> >> >> > + }
> >> >> > +}
