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 <pins...@gmail.com> wrote: > > On Fri, May 3, 2024 at 2:24 AM Aldy Hernandez <al...@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 > > >
From a022147e7a9a93d4fc8919aca77ed7fabc99eff7 Mon Sep 17 00:00:00 2001 From: Aldy Hernandez <al...@redhat.com> Date: Fri, 3 May 2024 11:17:32 +0200 Subject: [PATCH] [ranger] Force buffer alignment in Value_Range [PR114912] 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..f124dca34be 100644 --- a/gcc/value-range.h +++ b/gcc/value-range.h @@ -740,9 +740,13 @@ 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 buffer_type { + int_range_max ints; + frange floats; + unsupported_range unsupported; + buffer_type () { } + ~buffer_type () { } + } m_buffer; }; // The default constructor is uninitialized and must be initialized @@ -750,6 +754,7 @@ private: inline Value_Range::Value_Range () + : m_buffer () { m_vrange = NULL; } @@ -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 (&m_buffer.ints) int_range_max (); else if (frange::supports_p (type)) - m_vrange = new (&m_buffer) frange (); + m_vrange = new (&m_buffer.floats) frange (); else - m_vrange = new (&m_buffer) unsupported_range (); + m_vrange = new (&m_buffer.unsupported) 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 (&m_buffer.ints) 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 (&m_buffer.floats) frange (as_a <frange> (r)); else - m_vrange = new (&m_buffer) unsupported_range (as_a <unsupported_range> (r)); + m_vrange = new (&m_buffer.unsupported) + unsupported_range (as_a <unsupported_range> (r)); } // Assignment operator. Copying incompatible types is allowed. That -- 2.44.0