On Fri, Apr 7, 2017 at 5:01 PM, Nanley Chery <nanleych...@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.ch...@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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev