Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create MtlArray using memory allocated by Array #320

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

christiangnrd
Copy link
Contributor

@christiangnrd christiangnrd commented Mar 19, 2024

Feedback on implementation, interface, tests, etc. is welcome.

Closes #62

lib/mtl/buffer.jl Outdated Show resolved Hide resolved
lib/mtl/buffer.jl Outdated Show resolved Hide resolved
@christiangnrd christiangnrd marked this pull request as draft March 19, 2024 21:02
@christiangnrd christiangnrd added help wanted Extra attention is needed arrays Things about the array abstraction. labels Mar 19, 2024
@christiangnrd
Copy link
Contributor Author

Maybe a check to see if a pointer is page-aligned and throw otherwise? And then for unsafe_copyto! it could fallback to the old implementation if the pointer is not page-aligned?

@tgymnich
Copy link
Member

In theory the function is already unsafe. Might as well be up to the user to check? Not sure.

But yeah either documenting or asserting is a good idea.

@christiangnrd christiangnrd removed the help wanted Extra attention is needed label Mar 20, 2024
@christiangnrd christiangnrd force-pushed the newBufferNoCopy branch 21 times, most recently from d6ca650 to 5f246ae Compare March 20, 2024 13:51
@christiangnrd
Copy link
Contributor Author

I believe that this code works in MacOS 14.4 but not MacOS 13.x. For some reason, on 14.4, alloc_buffer_nobytes always works, even if the array is not page-aligned, while in MacOS 13, it always returns a null pointer.

Does anyone have any ideas as to why?

@maleadt
Copy link
Member

maleadt commented Mar 20, 2024

FWIW, https://developer.apple.com/documentation/metal/mtldevice/1433382-newbufferwithbytesnocopy still mentions that the pointer has to be page-aligned.

@christiangnrd
Copy link
Contributor Author

christiangnrd commented Mar 21, 2024

@maleadt
Copy link
Member

maleadt commented Apr 10, 2024

I think we always have to enforce pointer and buffer alignment:

pointer

    A page-aligned pointer to the starting memory address.
length

    The size of the new buffer, in bytes, that results in a page-aligned region of memory.

Even though it seems to work on macOS 14, it does seem to violate the docs. I'll push a commit, if you don't mind.

@maleadt
Copy link
Member

maleadt commented Apr 10, 2024

@christiangnrd I tightened the check as mentioned above, and also simplified the implementation a little: determining nocopy within unsafe_copyto so that it doesn't have to be passed as a kwarg that doesn't really make much sense IMO (because it's about the temporary buffer, which is an implementation detail to the unsafe_copyto! caller).

@christiangnrd
Copy link
Contributor Author

All the changes you made seem like improvements to me.

@maleadt maleadt merged commit 15b0ebf into JuliaGPU:main Apr 10, 2024
1 check passed
@maleadt
Copy link
Member

maleadt commented Apr 10, 2024

Thanks!

@christiangnrd christiangnrd deleted the newBufferNoCopy branch April 10, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays Things about the array abstraction.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to creating MtlArray using a memory allocated by Array
3 participants