Skip to content

Conversation

@ggerganov
Copy link
Member

Use std::sort in ggml_compute_forward_argsort_f32() instead of bubble sort.

@ggerganov ggerganov requested a review from slaren as a code owner November 12, 2025 15:12
@github-actions github-actions bot added the ggml changes relating to the ggml tensor library for machine learning label Nov 12, 2025
@ggerganov ggerganov merged commit 374fe09 into master Nov 12, 2025
71 of 72 checks passed
@ggerganov ggerganov deleted the gg/ggml-argsort-std branch November 12, 2025 18:43
Comment on lines +7694 to +7696
std::function<bool(int32_t, int32_t)> cmp;

// note: this might be causing memory allocations? ideally should be avoided if it's the case
Copy link
Member

Choose a reason for hiding this comment

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

Yeah std::function can be quite heavy, it is better to avoid it if possible.

Copy link
Member Author

@ggerganov ggerganov Nov 12, 2025

Choose a reason for hiding this comment

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

In the past I've used an alloc-free alternative of std::function (based on this), but it's quite cumbersome (nvm, not relevant for this use case). Will probably just implement qsort and avoid std::sort all together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both qsort() and std::function involves at least one indirect function call.
Perhaps better to simply inline the std::sort() call in the switch block as it's just an one-liner:

case GGML_SORT_ORDER_ASC: std::sort(dst_data, dst_data + ne0, [src_data](int32_t a, int32_t b) { return src_data[a] < src_data[b]; }); break;

This will specialize the std::sort template and avoids indirect function call.

Copy link
Member

Choose a reason for hiding this comment

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

An alternative would be to do something like this:

template<enum ggml_sort_order order>
struct sort_cmp {
    const float * data;
    bool operator()(int32_t a, int32_t b) const {
        if constexpr (order == GGML_SORT_ORDER_ASC) {
            return data[a] < data[b];
        } else {
            return data[a] > data[b];
        }
    }
};
switch (order) {
    case GGML_SORT_ORDER_ASC:
        std::sort(dst_data, dst_data + ne0, sort_cmp<GGML_SORT_ORDER_ASC>{src_data});
        break;

    case GGML_SORT_ORDER_DESC:
        std::sort(dst_data, dst_data + ne0, sort_cmp<GGML_SORT_ORDER_DESC>{src_data});
        break;

    default: GGML_ABORT("invalid sort order");
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ggml changes relating to the ggml tensor library for machine learning

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants