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

[Nanley Chery]

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 ?

> +    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.

> +
> +   /* 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.

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

> +   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