On Tue, 29 Jul 2025, Tamar Christina wrote:
> > -----Original Message-----
> > From: Pengfei Li <pengfei....@arm.com>
> > Sent: Tuesday, July 29, 2025 10:17 AM
> > To: Richard Biener <richard.guent...@gmail.com>; Tamar Christina
> > <tamar.christ...@arm.com>
> > Cc: gcc-patches@gcc.gnu.org; rguent...@suse.de; s...@gentoo.org
> > Subject: Re: [PATCH] vect: Fix insufficient alignment requirement for
> > speculative
> > loads [PR121190]
> >
> > Hi,
> >
> > Thanks Tamar and Richi for the review.
> >
> > > > I wonder about what the intention of this code was. It seems to me
> > > > that it was
> > > > trying to disable versioning for VLA, but then also doubling up and
> > > > using the
> > > > mode as the alignment. But the cross iteration alignment check below
> > > > this on
> > > > uses DR_TARGET_ALIGNMENT as one would expect, since the target alignment
> > > > can be different from the vector size.
> > > >
> > > > So I wonder if something like this isn't more appropriate:
> > > >
> > > > diff --git a/gcc/tree-vect-data-refs.cc b/gcc/tree-vect-data-refs.cc
> > > > index 75e06ff28e6..8595c76eae2 100644
> > > > --- a/gcc/tree-vect-data-refs.cc
> > > > +++ b/gcc/tree-vect-data-refs.cc
> > > > @@ -2972,7 +2972,8 @@ vect_enhance_data_refs_alignment (loop_vec_info
> > loop_vinfo)
> > > > VF is a power of two. We could relax this if we added
> > > > a way of enforcing a power-of-two size. */
> > > > unsigned HOST_WIDE_INT size;
> > > > - if (!GET_MODE_SIZE (TYPE_MODE (vectype)).is_constant
> > > > (&size))
> > > > + if (!GET_MODE_SIZE (TYPE_MODE (vectype)).is_constant ()
> > > > + || !DR_TARGET_ALIGNMENT (dr_info).is_constant (&size))
> > >
> > > I agree that checking DR_TARGET_ALIGNMENT is what needs to be done, I'm
> > > not
> > sure
> > > why we checked the size - probably historic. But there's no need to
> > > keep the size
> > > check, so just
> > >
> > > diff --git a/gcc/tree-vect-data-refs.cc b/gcc/tree-vect-data-refs.cc
> > > index b3ec0b67826..8d43b99a6f4 100644
> > > --- a/gcc/tree-vect-data-refs.cc
> > > +++ b/gcc/tree-vect-data-refs.cc
> > > @@ -2969,7 +2969,7 @@ vect_enhance_data_refs_alignment (loop_vec_info
> > > loop_vinfo)
> > > VF is a power of two. We could relax this if we added
> > > a way of enforcing a power-of-two size. */
> > > unsigned HOST_WIDE_INT size;
> > > - if (!GET_MODE_SIZE (TYPE_MODE (vectype)).is_constant
> > > (&size))
> > > + if (!DR_TARGET_ALIGNMENT (dr_info).is_constant (&size))
> > > {
> > > do_versioning = false;
> > > break;
> > >
> > > is correct. OK if that works, and sorry for the delay.
> >
> > I had compared the approach I used in the patch with the one Tamar
> > suggested.
> > Both have their pros and cons. I chose the one in my patch because I think
> > it
> > avoids unnecessarily increasing alignment requirements if no speculative
> > load
> > exists.
> >
> > But I would adopt your suggestion if you both think it would be better
> > overall.
> > I will update this and the test case, and post a new version soon.
target_alignment is what the target requires to make it aligned,
using the size is too pessimistic (and wrong in the early break case).
>
> The reason I still checked size is because if I'm not mistaken some VLA
> targets like SVE return that the desired alignment is the element size.
> So you'd get a non-poly constant there I believe, but we don't support
> peeling this was for VLA.
Yes, and using target_alignment will make it possible to use
alignment versioning in the first place (and SVE only requires
element alignment?)
Richard.
> Regards,
> Tamar
>
> > --
> > Thanks,
> > Pengfei
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
