[stdpar] Implement {m,aligned_}alloc and free

[nilsfriess]

No description provided.

[illuhad]

Great to see! Can you also update doc/stdpar.md? It says there in the "memory model" section that only new/delete are hijacked and malloc is not.

We should also look at malloc's other C-style friends like realloc and calloc, but that can be a separate PR :)

[nilsfriess]

Great to see! Can you also update doc/stdpar.md? It says there in the "memory model" section that only new/delete are hijacked and malloc is not.

Done :)

We should also look at malloc's other C-style friends like realloc and calloc, but that can be a separate PR :)

Yes definitely. I think calloc and aligned_alloc should be relatively easy to add, and I think the C standard also allows realloc to just free the old memory and allocate new memory instead of reusing the already allocated memory (if possible), so we could just do that.

[nilsfriess]

I now also added calloc and aligned_alloc. I think realloc is not that easy to implement because the standard says

The reallocation is done by [...] allocating a new memory block of size new_size bytes, copying memory area with size equal the lesser of the new and the old sizes, and freeing the old block.

That means it needs to know the size of the old allocation and I don't really know where to get that from

[illuhad]

One solution could be to allocate more memory than needed and letting the actual allocation follow a header that contains the size. When we need to realloc, we could then access the header by subtracting sizeof(header) from the user pointer. This seems to be how roc-stdpar does it:

https://github.com/ROCmSoftwarePlatform/roc-stdpar/blob/25e68a71e9f6d96478d440af9ea858540e448a3f/include/hipstdpar_lib.hpp#L167

However, I don't see code there to actually copy the data which might be an oversight.
I'm not sure if this is the best right solution, and whether the consequences are worth it (e.g. then you'd no longer have interoperability between sycl::malloc_shared/free and our hijacked memory functions, as you can no longer use a pointer with both).

I'd be fine leaving realloc unimplemented for now if you prefer.

[nilsfriess]

I think for now we can leave it unimplemented, I think it’s too much effort to implement it “just in case”.

[illuhad]

Three remarks here:

• I think here we definitely need a wait() call. Otherwise there's no guarantee the memset is complete by the time calloc returns the pointer.
• Also, it might be worth a look into whether hipsycl::algorithms::fill() from algorithms/algorithm.hpp might be a better fit here. It's not always best for performance to use queue::memset(), sometimes a queue::parallel_for() with a manual fill kernel is better. For example, on NUMA systems on CPU, parallel_for might better take locality into account. The fill() implementation has special code paths for these cases.
• Can you double check whether the disabled_stack condition is sufficient here? malloc has more complex conditions (disable_stack==0, usm_context::is_alive() (this prevents USM operations in the shutdown phase of the application), and a check on the pointer type (I assume this we can ignore because memset should only be called by calloc in the USM path, when we know that it is a USM pointer)

[nilsfriess]

I think here we definitely need a wait() call. Otherwise there's no guarantee the memset is complete by the time calloc returns the pointer.

Yup, that makes sense.

Also, it might be worth a look into whether hipsycl::algorithms::fill() from algorithms/algorithm.hpp might be a better fit here.

Yes that sounds like a better approach.

Can you double check whether the disabled_stack condition is sufficient here? malloc has more complex conditions (disable_stack==0, usm_context::is_alive())

malloc currently does not check whether usm_context::is_alive() (or am I missing something?), only free does, that's why I didn't add a check for that here. But I think I might redo this whole part a bit differently anyway and use malloc to allocate memory and use either hipsycl::algorithms::fill_n or libc's memset if malloc gave us a pointer with hipsycl::sycl::get_pointer_type == unknown to zero the memory region. So all allocations are then done using malloc and we don't need to reinvent the wheel here.

[nilsfriess]

I removed the calloc parts from this PR again as we've discussed.

[illuhad]

Thanks!