On Wed, Feb 12, 2014 at 04:28:14PM +0100, Richard Biener wrote:
> On Wed, Feb 12, 2014 at 4:21 PM, Trevor Saunders <tsaund...@mozilla.com>
> wrote:
> > On Wed, Feb 12, 2014 at 03:09:17PM +0100, Richard Biener wrote:
> >>
> >> The following aims to improve code generation for loops like
> >> those in tree-ssa-alias.c:
> >>
> >> auto_vec<tree, 16> component_refs1;
> >> auto_vec<tree, 16> component_refs2;
> >>
> >> /* Create the stack of handled components for REF1. */
> >> while (handled_component_p (ref1))
> >> {
> >> component_refs1.safe_push (ref1);
> >> ref1 = TREE_OPERAND (ref1, 0);
> >> }
> >>
> >> It does so by making sure the vector itself doesn't necessarily
> >> escape (to vec.c:calculate_allocation) and by simplifying the
> >> way we identify a vector using auto storage. In the end this
> >> results in us doing better CSE. But not SRA unfortunately,
> >> as we have
> >>
> >> component_refs1.m_header.m_alloc = 16;
> >> component_refs1.m_header.m_using_auto_storage = 1;
> >> component_refs1.m_header.m_num = 0;
> >> component_refs1.D.56915.m_vec = &component_refs1.m_header;
> >>
> >> which is what keeps component_refs1 address-taken (no easy
> >> way out of that I fear).
> >>
> >> It also gets rid of the aliasing-suspicious
> >>
> >> this->m_vec = reinterpret_cast<vec<T, va_heap, vl_embed> *>
> >> (&m_header);
> >>
> >> and the friend declaration in class auto_vec.
> >
> > over all I like this, only two small comments
> >
> >> (vec_prefix::m_has_auto_buf): Rename to ...
> >> (vec_prefix::m_using_auto_storage): ... this.
> >
> > One optimization that might be worth doing is if a vector is cleared
> > reseting m_vec to point at the internal storage in which case the old
> > name would be more accurate, on the other hand its probably rare that's
> > useful, and doing it this way is simpler.
>
> Yeah, I thought about this but disregarded it as not-the-common case.
makes sense.
> >> ~auto_vec ()
> >> --- 1250,1257 ----
> >> public:
> >> auto_vec ()
> >> {
> >> ! m_auto.embedded_init (MAX (N, 2), 0, 1);
> >> ! this->m_vec = &m_auto;
> >> }
> >>
> >> ~auto_vec ()
> >> *************** public:
> >> *** 1246,1255 ****
> >> }
> >>
> >> private:
> >> ! friend class vec<T, va_heap, vl_ptr>;
> >> !
> >> ! vec_prefix m_header;
> >> ! T m_data[N];
> >> };
> >>
> >> /* auto_vec is a sub class of vec whose storage is released when it is
> >> --- 1260,1267 ----
> >> }
> >>
> >> private:
> >> ! vec<T, va_heap, vl_embed> m_auto;
> >> ! T m_data[MAX (N - 1, 1)];
> >
> > why is the MAX() needed? auto_vec<T, 0> is specialized below so someone
> > would need to write auto_vec<T, -x> for N - 1 to be less than 1 which is
> > crazy, and seems like shouldn't be allowed to compile.
>
> It's for the quite common auto_vec<T, 1> case where T m_data[N-1]
> doesn't work (zero-sized array is a GNU extension). I've had the
> variant of simply allocating N + 1 elements (and thus using
> m_data[N]) but then the above is closer to what people expect (but
> sets the minimum auto size to two elements).
I forgot about and then missed embedded vectors having the one internal
element, so yeah this makes sense.
> Alternatively one could have templated vec<T, va_heap, vl_embed>
yeah, lets not do that its already way too template happy.
> itself with a size for its m_data member. Or specialize auto_vec
> for N == 1 and leave out the m_data member for that.
>
> I considered both not worth the trouble ;)
sgtm
Thanks!
Trev
>
> If you prefer using m_data[N] and thus allocating one more element
> than requested in auto storage I can do that as well.
>
> Thanks,
> Richard.
>
> > thanks!
> >
> > Trev
> >
> >> };
> >>
> >> /* auto_vec is a sub class of vec whose storage is released when it is
> >> *************** template<typename T>
> >> *** 1396,1402 ****
> >> inline bool
> >> vec<T, va_heap, vl_ptr>::reserve (unsigned nelems, bool exact
> >> MEM_STAT_DECL)
> >> {
> >> ! if (!nelems || space (nelems))
> >> return false;
> >>
> >> /* For now play a game with va_heap::reserve to hide our auto storage
> >> if any,
> >> --- 1408,1414 ----
> >> inline bool
> >> vec<T, va_heap, vl_ptr>::reserve (unsigned nelems, bool exact
> >> MEM_STAT_DECL)
> >> {
> >> ! if (space (nelems))
> >> return false;
> >>
> >> /* For now play a game with va_heap::reserve to hide our auto storage
> >> if any,
> >> *************** vec<T, va_heap, vl_ptr>::release (void)
> >> *** 1462,1468 ****
> >>
> >> if (using_auto_storage ())
> >> {
> >> ! static_cast<auto_vec<T, 1> *> (this)->m_header.m_num = 0;
> >> return;
> >> }
> >>
> >> --- 1474,1480 ----
> >>
> >> if (using_auto_storage ())
> >> {
> >> ! m_vec->m_vecpfx.m_num = 0;
> >> return;
> >> }
> >>
> >> *************** template<typename T>
> >> *** 1705,1716 ****
> >> inline bool
> >> vec<T, va_heap, vl_ptr>::using_auto_storage () const
> >> {
> >> ! if (!m_vec->m_vecpfx.m_has_auto_buf)
> >> ! return false;
> >> !
> >> ! const vec_prefix *auto_header
> >> ! = &static_cast<const auto_vec<T, 1> *> (this)->m_header;
> >> ! return reinterpret_cast<vec_prefix *> (m_vec) == auto_header;
> >> }
> >>
> >> #if (GCC_VERSION >= 3000)
> >> --- 1717,1723 ----
> >> inline bool
> >> vec<T, va_heap, vl_ptr>::using_auto_storage () const
> >> {
> >> ! return m_vec->m_vecpfx.m_using_auto_storage;
> >> }
> >>
> >> #if (GCC_VERSION >= 3000)
