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

Add support for setting the default allocator and deallocator functions in Halide::Runtime::Buffer. #8132

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

mcourteaux
Copy link
Contributor

@mcourteaux mcourteaux commented Mar 1, 2024

This uses static inline to use inline linkage, as suggested by @abadams, to deduplicate upon link time the storage of the struct holding the two function pointers.

Outdated:

Additionally, the struct is templated and parameterized over the Runtime::Buffer template argument to make sure that the storage is unique to the Buffer<void> struct (and thus does not gets linked in for every type/dim combo of Buffer.

One option is to change the setters from this:

    static void set_default_allocate_fn(void *(*allocate_fn)(size_t)) {
        Buffer<>::default_allocator_fns.default_allocate_fn = allocate_fn;
    }
    static void set_default_deallocate_fn(void (*deallocate_fn)(void *)) {
        Buffer<>::default_allocator_fns.default_deallocate_fn = deallocate_fn;
    }

to this:

    static void set_default_allocate_fn(void *(*allocate_fn)(size_t)) {
        default_allocator_fns.default_allocate_fn = allocate_fn;
    }
    static void set_default_deallocate_fn(void (*deallocate_fn)(void *)) {
        default_allocator_fns.default_deallocate_fn = deallocate_fn;
    }

This way, it will only be valid to try to set them on the Buffer<> or Buffer<void> template instance. I.e., it will fail on Buffer<float>::set_default_allocate_fn(), due to the specialization of the DefaultAllocatorFn<> struct.

Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add a test :-)

@abadams
Copy link
Member

abadams commented Mar 4, 2024

The place to add it would be in test/correctness/halide_buffer.cpp

@mcourteaux
Copy link
Contributor Author

Was also thinking that I don't see a reason to embed this struct in the Buffer struct. We can make those two fields static inline right in the auxiliary struct.

@mcourteaux
Copy link
Contributor Author

It's ready for merge, IMO.

@steven-johnson steven-johnson self-requested a review March 5, 2024 16:15
@steven-johnson steven-johnson merged commit 8b3312c into halide:main Mar 5, 2024
18 checks passed
@abadams abadams added the release_notes For changes that may warrant a note in README for official releases. label Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_notes For changes that may warrant a note in README for official releases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants