[Mesa-dev] [PATCH v3 2/6] anv: Add a helper for doing mass allocations

[Jason Ekstrand]

On Fri, Apr 7, 2017 at 5:01 PM, Nanley Chery <nanleychery at gmail.com> wrote:

> On Fri, Apr 07, 2017 at 12:55:53PM -0700, Jason Ekstrand wrote:
> > We tend to try to reduce the number of allocation calls the Vulkan
> > driver uses by doing a single allocation whenever possible for a data
> > structure.  While this has certain downsides (usually code complexity),
> > it does mean error handling and cleanup is much easier.  This commit
> > adds a nice little helper struct for getting rid of some of that
> > complexity.
> > ---
> >  src/intel/vulkan/anv_private.h | 77 ++++++++++++++++++++++++++++++
> ++++++++++++
> >  1 file changed, 77 insertions(+)
> >
> > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> > index 90974d9..f941cc7 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -269,6 +269,83 @@ void anv_loge_v(const char *format, va_list va);
> >  #define anv_assert(x)
> >  #endif
> >
> > +struct anv_multialloc {
> > +    size_t size;
> > +    size_t align;
> > +
> > +    uint32_t ptr_count;
>
> uint8_t ?
>

We could, sure.  But it won't use any less space since it's sandwiched
between a size_t and a pointer.  Also, it *should* just go away.


> > +    void **ptrs[8];
> > +};
> > +
> > +#define ANV_MULTIALLOC_INIT \
> > +   ((struct anv_multialloc) { 0, })
> > +
> > +#define ANV_MULTIALLOC(_name) \
> > +   struct anv_multialloc _name = ANV_MULTIALLOC_INIT
> > +
> > +static inline void
> > +_anv_multialloc_add(struct anv_multialloc *ma,
> > +                    void **ptr, size_t size, size_t align)
> > +{
> > +   size_t offset = align_u64(ma->size, align);
> > +   ma->size = offset + size;
> > +   ma->align = MAX2(ma->align, align);
>
> Why do we align the allocation to the maximum alignment? Every pointer
> except the first is offset according to its structure's alignment. It
> seems like we only need to save off the first structure's alignment.
>

ma->align is what we pass into the allocator to tell it how to align the
base pointer.  If we want a pointer to have a particular alignment, we need
both its offset from the base pointer to have that alignment and the base
pointer needs to have at least that alignment.


> > +
> > +   /* Store the offset in the pointer. */
> > +   *ptr = (void *)(uintptr_t)offset;
> > +
> > +   assert(ma->ptr_count < ARRAY_SIZE(ma->ptrs));
> > +   ma->ptrs[ma->ptr_count++] = ptr;
> > +}
> > +
> > +#define anv_multialloc_add(_ma, _ptr, _count) \
> > +   _anv_multialloc_add((_ma), (void **)(_ptr), \
> > +                       (_count) * sizeof(**(_ptr)),
> __alignof__(**(_ptr)))
> > +
> > +static inline void *
> > +anv_multialloc_alloc(struct anv_multialloc *ma,
> > +                     const VkAllocationCallbacks *alloc,
> > +                     VkSystemAllocationScope scope)
> > +{
> > +   void *ptr = vk_alloc(alloc, ma->size, ma->align, scope);
> > +   if (!ptr)
> > +      return NULL;
> > +
> > +   /* Fill out each of the pointers with their final value.
> > +    *
> > +    *   for (uint32_t i = 0; i < ma->ptr_count; i++)
> > +    *      *ma->ptrs[i] = ptr + (uintptr_t)*ma->ptrs[i];
> > +    *
> > +    * Unfortunately, even though ma->ptr_count is basically guaranteed
> to be a
> > +    * constant, GCC is incapable of figuring this out and unrolling the
> loop
> > +    * so we have to give it a little help.
> > +    */
>
> Is GCC not able to figure this out if the ma parameter was qualified
> with 'restrict'? Maybe it thinks that *ma->ptrs[_i] may refer to
> ma->ptr_count.
>

Nope.  Doesn't help.  I also tried

for (uint32_t i = 0; i < ARRAY_SIZE(ma->ptrs); i++) {
   if (i < ma->ptr_count)
      *ma->ptrs[i] = ptr + (uintptr_t)*ma->ptrs[i];
}

and it can't figure that one out either.


> I had some nitpicks, but nonetheless this series is
> Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>
>

Thanks!


> > +   STATIC_ASSERT(ARRAY_SIZE(ma->ptrs) == 8);
> > +#define _ANV_MULTIALLOC_UPDATE_POINTER(_i) \
> > +   if ((_i) < ma->ptr_count) \
> > +      *ma->ptrs[_i] = ptr + (uintptr_t)*ma->ptrs[_i]
> > +   _ANV_MULTIALLOC_UPDATE_POINTER(0);
> > +   _ANV_MULTIALLOC_UPDATE_POINTER(1);
> > +   _ANV_MULTIALLOC_UPDATE_POINTER(2);
> > +   _ANV_MULTIALLOC_UPDATE_POINTER(3);
> > +   _ANV_MULTIALLOC_UPDATE_POINTER(4);
> > +   _ANV_MULTIALLOC_UPDATE_POINTER(5);
> > +   _ANV_MULTIALLOC_UPDATE_POINTER(6);
> > +   _ANV_MULTIALLOC_UPDATE_POINTER(7);
> > +#undef _ANV_MULTIALLOC_UPDATE_POINTER
> > +
> > +   return ptr;
> > +}
> > +
> > +static inline void *
> > +anv_multialloc_alloc2(struct anv_multialloc *ma,
> > +                      const VkAllocationCallbacks *parent_alloc,
> > +                      const VkAllocationCallbacks *alloc,
> > +                      VkSystemAllocationScope scope)
> > +{
> > +   return anv_multialloc_alloc(ma, alloc ? alloc : parent_alloc, scope);
> > +}
> > +
> >  /**
> >   * A dynamically growable, circular buffer.  Elements are added at head
> and
> >   * removed from tail. head and tail are free-running uint32_t indices
> and we
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170407/dc0ef775/attachment-0001.html>