<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Apr 7, 2017 at 5:01 PM, Nanley Chery <span dir="ltr"><<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On Fri, Apr 07, 2017 at 12:55:53PM -0700, Jason Ekstrand wrote:<br>
</span><span class="gmail-">> We tend to try to reduce the number of allocation calls the Vulkan<br>
> driver uses by doing a single allocation whenever possible for a data<br>
> structure. While this has certain downsides (usually code complexity),<br>
> it does mean error handling and cleanup is much easier. This commit<br>
> adds a nice little helper struct for getting rid of some of that<br>
> complexity.<br>
> ---<br>
> src/intel/vulkan/anv_private.h | 77 ++++++++++++++++++++++++++++++<wbr>++++++++++++<br>
> 1 file changed, 77 insertions(+)<br>
><br>
> diff --git a/src/intel/vulkan/anv_<wbr>private.h b/src/intel/vulkan/anv_<wbr>private.h<br>
> index 90974d9..f941cc7 100644<br>
> --- a/src/intel/vulkan/anv_<wbr>private.h<br>
> +++ b/src/intel/vulkan/anv_<wbr>private.h<br>
> @@ -269,6 +269,83 @@ void anv_loge_v(const char *format, va_list va);<br>
> #define anv_assert(x)<br>
> #endif<br>
><br>
> +struct anv_multialloc {<br>
> + size_t size;<br>
> + size_t align;<br>
> +<br>
> + uint32_t ptr_count;<br>
<br>
</span>uint8_t ?<span class="gmail-"><br></span></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> + void **ptrs[8];<br>
> +};<br>
> +<br>
> +#define ANV_MULTIALLOC_INIT \<br>
> + ((struct anv_multialloc) { 0, })<br>
> +<br>
> +#define ANV_MULTIALLOC(_name) \<br>
> + struct anv_multialloc _name = ANV_MULTIALLOC_INIT<br>
> +<br>
> +static inline void<br>
> +_anv_multialloc_add(struct anv_multialloc *ma,<br>
> + void **ptr, size_t size, size_t align)<br>
> +{<br>
> + size_t offset = align_u64(ma->size, align);<br>
> + ma->size = offset + size;<br>
> + ma->align = MAX2(ma->align, align);<br>
<br>
</span>Why do we align the allocation to the maximum alignment? Every pointer<br>
except the first is offset according to its structure's alignment. It<br>
seems like we only need to save off the first structure's alignment.<br><div><div class="gmail-h5"></div></div></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5">
> +<br>
> + /* Store the offset in the pointer. */<br>
> + *ptr = (void *)(uintptr_t)offset;<br>
> +<br>
> + assert(ma->ptr_count < ARRAY_SIZE(ma->ptrs));<br>
> + ma->ptrs[ma->ptr_count++] = ptr;<br>
> +}<br>
> +<br>
> +#define anv_multialloc_add(_ma, _ptr, _count) \<br>
> + _anv_multialloc_add((_ma), (void **)(_ptr), \<br>
> + (_count) * sizeof(**(_ptr)), __alignof__(**(_ptr)))<br>
> +<br>
> +static inline void *<br>
> +anv_multialloc_alloc(struct anv_multialloc *ma,<br>
> + const VkAllocationCallbacks *alloc,<br>
> + VkSystemAllocationScope scope)<br>
> +{<br>
> + void *ptr = vk_alloc(alloc, ma->size, ma->align, scope);<br>
> + if (!ptr)<br>
> + return NULL;<br>
> +<br>
> + /* Fill out each of the pointers with their final value.<br>
> + *<br>
> + * for (uint32_t i = 0; i < ma->ptr_count; i++)<br>
> + * *ma->ptrs[i] = ptr + (uintptr_t)*ma->ptrs[i];<br>
> + *<br>
> + * Unfortunately, even though ma->ptr_count is basically guaranteed to be a<br>
> + * constant, GCC is incapable of figuring this out and unrolling the loop<br>
> + * so we have to give it a little help.<br>
> + */<br>
<br>
</div></div>Is GCC not able to figure this out if the ma parameter was qualified<br>
with 'restrict'? Maybe it thinks that *ma->ptrs[_i] may refer to<br>
ma->ptr_count.<br></blockquote><div><br></div><div>Nope. Doesn't help. I also tried<br><br></div><div>for (uint32_t i = 0; i < ARRAY_SIZE(ma->ptrs); i++) {<br></div><div> if (i < ma->ptr_count)<br> *ma->ptrs[i] = ptr + (uintptr_t)*ma->ptrs[i];</div><div>}<br><br></div><div>and it can't figure that one out either.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I had some nitpicks, but nonetheless this series is<br>
Reviewed-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>><br><div><div class="gmail-h5"></div></div></blockquote><div><br></div><div>Thanks!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5">
> + STATIC_ASSERT(ARRAY_SIZE(ma-><wbr>ptrs) == 8);<br>
> +#define _ANV_MULTIALLOC_UPDATE_<wbr>POINTER(_i) \<br>
> + if ((_i) < ma->ptr_count) \<br>
> + *ma->ptrs[_i] = ptr + (uintptr_t)*ma->ptrs[_i]<br>
> + _ANV_MULTIALLOC_UPDATE_<wbr>POINTER(0);<br>
> + _ANV_MULTIALLOC_UPDATE_<wbr>POINTER(1);<br>
> + _ANV_MULTIALLOC_UPDATE_<wbr>POINTER(2);<br>
> + _ANV_MULTIALLOC_UPDATE_<wbr>POINTER(3);<br>
> + _ANV_MULTIALLOC_UPDATE_<wbr>POINTER(4);<br>
> + _ANV_MULTIALLOC_UPDATE_<wbr>POINTER(5);<br>
> + _ANV_MULTIALLOC_UPDATE_<wbr>POINTER(6);<br>
> + _ANV_MULTIALLOC_UPDATE_<wbr>POINTER(7);<br>
> +#undef _ANV_MULTIALLOC_UPDATE_POINTER<br>
> +<br>
> + return ptr;<br>
> +}<br>
> +<br>
> +static inline void *<br>
> +anv_multialloc_alloc2(struct anv_multialloc *ma,<br>
> + const VkAllocationCallbacks *parent_alloc,<br>
> + const VkAllocationCallbacks *alloc,<br>
> + VkSystemAllocationScope scope)<br>
> +{<br>
> + return anv_multialloc_alloc(ma, alloc ? alloc : parent_alloc, scope);<br>
> +}<br>
> +<br>
> /**<br>
> * A dynamically growable, circular buffer. Elements are added at head and<br>
> * removed from tail. head and tail are free-running uint32_t indices and we<br>
> --<br>
> 2.5.0.400.gff86faf<br>
><br>
</div></div>> ______________________________<wbr>_________________<br>
> mesa-dev mailing list<br>
> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</blockquote></div><br></div></div>