Re: [PATCH] Improve vec<auto, N> code generation

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Feb 12, 2014 at 4:21 PM, Trevor Saunders <tsaunders@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.

>>     ~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).

Alternatively one could have templated vec<T, va_heap, vl_embed>
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 ;)

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)