Skip to content

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 mcourteaux force-pushed the buffer_custom_alloc branch from 027b6a2 to 435194d Compare March 5, 2024 11:03
@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
@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