[ranger] Force buffer alignment in Value_Range [PR114912]

Sparc requires strict alignment and is choking on the byte vector in
Value_Range.  Is this the right approach, or is there a more canonical
way of forcing alignment?

If this is correct, OK for trunk?

gcc/ChangeLog:

	* value-range.h (class Value_Range): Use a union.
---
 gcc/value-range.h | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

On Fri, May 3, 2024 at 2:24 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> Sparc requires strict alignment and is choking on the byte vector in
> Value_Range.  Is this the right approach, or is there a more canonical
> way of forcing alignment?

I think the suggestion was to change over to use an union and use the
types directly in the union (anonymous unions and unions containing
non-PODs are part of C++11).
That is:
union {
  int_range_max int_range;
  frange fload_range;
  unsupported_range un_range;
};
...
m_vrange = new (&int_range) int_range_max ();
...

Also the canonical way of forcing alignment in C++ is to use aliagnas
as my patch in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114912
did.
Also I suspect the alignment is not word alignment but rather the
alignment of HOST_WIDE_INT which is not always the same as the
alignment of the pointer but bigger and that is why it is failing on
sparc (32bit rather than 64bit).

Thanks,
Andrew Pinski

>
> If this is correct, OK for trunk?
>
> gcc/ChangeLog:
>
>         * value-range.h (class Value_Range): Use a union.
> ---
>  gcc/value-range.h | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/value-range.h b/gcc/value-range.h
> index 934eec9e386..31af7888018 100644
> --- a/gcc/value-range.h
> +++ b/gcc/value-range.h
> @@ -740,9 +740,14 @@ private:
>    void init (const vrange &);
>
>    vrange *m_vrange;
> -  // The buffer must be at least the size of the largest range.
> -  static_assert (sizeof (int_range_max) > sizeof (frange), "");
> -  char m_buffer[sizeof (int_range_max)];
> +  union {
> +    // The buffer must be at least the size of the largest range, and
> +    // be aligned on a word boundary for strict alignment targets
> +    // such as sparc.
> +    static_assert (sizeof (int_range_max) > sizeof (frange), "");
> +    char m_buffer[sizeof (int_range_max)];
> +    void *align;
> +  } u;
>  };
>
>  // The default constructor is uninitialized and must be initialized
> @@ -816,11 +821,11 @@ Value_Range::init (tree type)
>    gcc_checking_assert (TYPE_P (type));
>
>    if (irange::supports_p (type))
> -    m_vrange = new (&m_buffer) int_range_max ();
> +    m_vrange = new (&u.m_buffer) int_range_max ();
>    else if (frange::supports_p (type))
> -    m_vrange = new (&m_buffer) frange ();
> +    m_vrange = new (&u.m_buffer) frange ();
>    else
> -    m_vrange = new (&m_buffer) unsupported_range ();
> +    m_vrange = new (&u.m_buffer) unsupported_range ();
>  }
>
>  // Initialize object with a copy of R.
> @@ -829,11 +834,12 @@ inline void
>  Value_Range::init (const vrange &r)
>  {
>    if (is_a <irange> (r))
> -    m_vrange = new (&m_buffer) int_range_max (as_a <irange> (r));
> +    m_vrange = new (&u.m_buffer) int_range_max (as_a <irange> (r));
>    else if (is_a <frange> (r))
> -    m_vrange = new (&m_buffer) frange (as_a <frange> (r));
> +    m_vrange = new (&u.m_buffer) frange (as_a <frange> (r));
>    else
> -    m_vrange = new (&m_buffer) unsupported_range (as_a <unsupported_range> (r));
> +    m_vrange
> +      = new (&u.m_buffer) unsupported_range (as_a <unsupported_range> (r));
>  }
>
>  // Assignment operator.  Copying incompatible types is allowed.  That
> --
> 2.44.0
>

Ahh, that is indeed cleaner, and there's no longer a need to assert
the sizeof of individual ranges.

It looks like a default constructor is needed for the buffer now, but
only for the default constructor of Value_Range.

I have verified that the individual range constructors are not called
on initialization to Value_Range, which was the original point of the
patch.  I have also run our performance suite, and there are no
changes to VRP or overall.

I would appreciate a review from someone more C++ savvy than me :).

OK for trunk?

On Fri, May 3, 2024 at 11:32 AM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Fri, May 3, 2024 at 2:24 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> > Sparc requires strict alignment and is choking on the byte vector in
> > Value_Range.  Is this the right approach, or is there a more canonical
> > way of forcing alignment?
>
> I think the suggestion was to change over to use an union and use the
> types directly in the union (anonymous unions and unions containing
> non-PODs are part of C++11).
> That is:
> union {
>   int_range_max int_range;
>   frange fload_range;
>   unsupported_range un_range;
> };
> ...
> m_vrange = new (&int_range) int_range_max ();
> ...
>
> Also the canonical way of forcing alignment in C++ is to use aliagnas
> as my patch in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114912
> did.
> Also I suspect the alignment is not word alignment but rather the
> alignment of HOST_WIDE_INT which is not always the same as the
> alignment of the pointer but bigger and that is why it is failing on
> sparc (32bit rather than 64bit).
>
> Thanks,
> Andrew Pinski
>
> >
> > If this is correct, OK for trunk?
> >
> > gcc/ChangeLog:
> >
> >         * value-range.h (class Value_Range): Use a union.
> > ---
> >  gcc/value-range.h | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/gcc/value-range.h b/gcc/value-range.h
> > index 934eec9e386..31af7888018 100644
> > --- a/gcc/value-range.h
> > +++ b/gcc/value-range.h
> > @@ -740,9 +740,14 @@ private:
> >    void init (const vrange &);
> >
> >    vrange *m_vrange;
> > -  // The buffer must be at least the size of the largest range.
> > -  static_assert (sizeof (int_range_max) > sizeof (frange), "");
> > -  char m_buffer[sizeof (int_range_max)];
> > +  union {
> > +    // The buffer must be at least the size of the largest range, and
> > +    // be aligned on a word boundary for strict alignment targets
> > +    // such as sparc.
> > +    static_assert (sizeof (int_range_max) > sizeof (frange), "");
> > +    char m_buffer[sizeof (int_range_max)];
> > +    void *align;
> > +  } u;
> >  };
> >
> >  // The default constructor is uninitialized and must be initialized
> > @@ -816,11 +821,11 @@ Value_Range::init (tree type)
> >    gcc_checking_assert (TYPE_P (type));
> >
> >    if (irange::supports_p (type))
> > -    m_vrange = new (&m_buffer) int_range_max ();
> > +    m_vrange = new (&u.m_buffer) int_range_max ();
> >    else if (frange::supports_p (type))
> > -    m_vrange = new (&m_buffer) frange ();
> > +    m_vrange = new (&u.m_buffer) frange ();
> >    else
> > -    m_vrange = new (&m_buffer) unsupported_range ();
> > +    m_vrange = new (&u.m_buffer) unsupported_range ();
> >  }
> >
> >  // Initialize object with a copy of R.
> > @@ -829,11 +834,12 @@ inline void
> >  Value_Range::init (const vrange &r)
> >  {
> >    if (is_a <irange> (r))
> > -    m_vrange = new (&m_buffer) int_range_max (as_a <irange> (r));
> > +    m_vrange = new (&u.m_buffer) int_range_max (as_a <irange> (r));
> >    else if (is_a <frange> (r))
> > -    m_vrange = new (&m_buffer) frange (as_a <frange> (r));
> > +    m_vrange = new (&u.m_buffer) frange (as_a <frange> (r));
> >    else
> > -    m_vrange = new (&m_buffer) unsupported_range (as_a <unsupported_range> (r));
> > +    m_vrange
> > +      = new (&u.m_buffer) unsupported_range (as_a <unsupported_range> (r));
> >  }
> >
> >  // Assignment operator.  Copying incompatible types is allowed.  That
> > --
> > 2.44.0
> >
>

Pushed to trunk to unblock sparc.


On Fri, May 3, 2024 at 4:24 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> Ahh, that is indeed cleaner, and there's no longer a need to assert
> the sizeof of individual ranges.
>
> It looks like a default constructor is needed for the buffer now, but
> only for the default constructor of Value_Range.
>
> I have verified that the individual range constructors are not called
> on initialization to Value_Range, which was the original point of the
> patch.  I have also run our performance suite, and there are no
> changes to VRP or overall.
>
> I would appreciate a review from someone more C++ savvy than me :).
>
> OK for trunk?
>
> On Fri, May 3, 2024 at 11:32 AM Andrew Pinski <pinskia@gmail.com> wrote:
> >
> > On Fri, May 3, 2024 at 2:24 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> > >
> > > Sparc requires strict alignment and is choking on the byte vector in
> > > Value_Range.  Is this the right approach, or is there a more canonical
> > > way of forcing alignment?
> >
> > I think the suggestion was to change over to use an union and use the
> > types directly in the union (anonymous unions and unions containing
> > non-PODs are part of C++11).
> > That is:
> > union {
> >   int_range_max int_range;
> >   frange fload_range;
> >   unsupported_range un_range;
> > };
> > ...
> > m_vrange = new (&int_range) int_range_max ();
> > ...
> >
> > Also the canonical way of forcing alignment in C++ is to use aliagnas
> > as my patch in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114912
> > did.
> > Also I suspect the alignment is not word alignment but rather the
> > alignment of HOST_WIDE_INT which is not always the same as the
> > alignment of the pointer but bigger and that is why it is failing on
> > sparc (32bit rather than 64bit).
> >
> > Thanks,
> > Andrew Pinski
> >
> > >
> > > If this is correct, OK for trunk?
> > >
> > > gcc/ChangeLog:
> > >
> > >         * value-range.h (class Value_Range): Use a union.
> > > ---
> > >  gcc/value-range.h | 24 +++++++++++++++---------
> > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/gcc/value-range.h b/gcc/value-range.h
> > > index 934eec9e386..31af7888018 100644
> > > --- a/gcc/value-range.h
> > > +++ b/gcc/value-range.h
> > > @@ -740,9 +740,14 @@ private:
> > >    void init (const vrange &);
> > >
> > >    vrange *m_vrange;
> > > -  // The buffer must be at least the size of the largest range.
> > > -  static_assert (sizeof (int_range_max) > sizeof (frange), "");
> > > -  char m_buffer[sizeof (int_range_max)];
> > > +  union {
> > > +    // The buffer must be at least the size of the largest range, and
> > > +    // be aligned on a word boundary for strict alignment targets
> > > +    // such as sparc.
> > > +    static_assert (sizeof (int_range_max) > sizeof (frange), "");
> > > +    char m_buffer[sizeof (int_range_max)];
> > > +    void *align;
> > > +  } u;
> > >  };
> > >
> > >  // The default constructor is uninitialized and must be initialized
> > > @@ -816,11 +821,11 @@ Value_Range::init (tree type)
> > >    gcc_checking_assert (TYPE_P (type));
> > >
> > >    if (irange::supports_p (type))
> > > -    m_vrange = new (&m_buffer) int_range_max ();
> > > +    m_vrange = new (&u.m_buffer) int_range_max ();
> > >    else if (frange::supports_p (type))
> > > -    m_vrange = new (&m_buffer) frange ();
> > > +    m_vrange = new (&u.m_buffer) frange ();
> > >    else
> > > -    m_vrange = new (&m_buffer) unsupported_range ();
> > > +    m_vrange = new (&u.m_buffer) unsupported_range ();
> > >  }
> > >
> > >  // Initialize object with a copy of R.
> > > @@ -829,11 +834,12 @@ inline void
> > >  Value_Range::init (const vrange &r)
> > >  {
> > >    if (is_a <irange> (r))
> > > -    m_vrange = new (&m_buffer) int_range_max (as_a <irange> (r));
> > > +    m_vrange = new (&u.m_buffer) int_range_max (as_a <irange> (r));
> > >    else if (is_a <frange> (r))
> > > -    m_vrange = new (&m_buffer) frange (as_a <frange> (r));
> > > +    m_vrange = new (&u.m_buffer) frange (as_a <frange> (r));
> > >    else
> > > -    m_vrange = new (&m_buffer) unsupported_range (as_a <unsupported_range> (r));
> > > +    m_vrange
> > > +      = new (&u.m_buffer) unsupported_range (as_a <unsupported_range> (r));
> > >  }
> > >
> > >  // Assignment operator.  Copying incompatible types is allowed.  That
> > > --
> > > 2.44.0
> > >
> >