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

ggml : improve ggml_is_contiguous logic #7856

Merged
merged 2 commits into from
Jun 12, 2024
Merged

ggml : improve ggml_is_contiguous logic #7856

merged 2 commits into from
Jun 12, 2024

Conversation

ggerganov
Copy link
Owner

@ggerganov ggerganov commented Jun 10, 2024

With this change, tensors with the following dimensions will now be considered contiguous:

# example 0
type = GGML_TYPE_F32
ne = ([0] = 2, [1] = 1, [2] = 1, [3] = 1)
nb = ([0] = 4, [1] = 8, [2] = 9999, [3] = 9999)

# example 1
type = GGML_TYPE_F32
ne = ([0] = 1, [1] = 25, [2] = 1, [3] = 1)
nb = ([0] = 100, [1] = 4, [2] = 100, [3] = 100)

# example 2
type = GGML_TYPE_Q8_0
ne = ([0] = 32,   [1] = 1, [2] = 1, [3] = 1)
nb = ([0] = 9999, [1] = 9999, [2] = 9999, [3] = 9999)

The logic being that if a dimension ne[d] == 1 then the stride is allowed to be arbitrary because we will never use it to offset a ptr (i.e. id * nb[d] will always be 0 because id loops in [0, 1), i.e. it is always 0)

This needs careful review as I'm not 100% confident about the impact. Started looking into this after trying to assert correctly the unary op implementations in #7857

@ggerganov ggerganov requested a review from slaren June 10, 2024 14:15
@github-actions github-actions bot added the ggml changes relating to the ggml tensor library for machine learning label Jun 10, 2024
@slaren
Copy link
Collaborator

slaren commented Jun 10, 2024

This seems very unlikely, but this check would detect a case such as this as contiguous, right?

type = GGML_TYPE_F32
ne = ([0] = 2, [1] = 1, [2] = 1, [3] = 2)
nb = ([0] = 4, [1] = 8, [2] = 9999, [3] = 9999)

@slaren
Copy link
Collaborator

slaren commented Jun 10, 2024

How about something like this (untested):

static bool ggml_is_contiguous_n(const struct ggml_tensor * tensor, int n) {
    size_t next_nb = ggml_type_size(tensor->type);
    if (tensor->ne[0] != ggml_blck_size(tensor->type) && tensor->nb[0] != next_nb) {
        return false;
    }
    next_nb *= tensor->ne[0]/ggml_blck_size(tensor->type);
    for (int i = 1; i < GGML_MAX_DIMS; i++) {
        if (tensor->ne[i] != 1) {
            if (i > n) {
                if (tensor->nb[i] != next_nb) {
                    return false;
                }
                next_nb *= tensor->ne[i];
            }
            else {
                // this dimension does not need to be contiguous
                next_nb = tensor->ne[i]*tensor->nb[i];
            }
        }
    }
    return true;
}

GGML_CALL bool ggml_is_contiguous(const struct ggml_tensor * tensor) {
    return ggml_is_contiguous_0(tensor);
}

GGML_CALL bool ggml_is_contiguous_0(const struct ggml_tensor * tensor) {
    return ggml_is_contiguous_n(tensor, 0);
}

GGML_CALL bool ggml_is_contiguous_1(const struct ggml_tensor * tensor) {
    return ggml_is_contiguous_n(tensor, 1);
}

GGML_CALL bool ggml_is_contiguous_2(const struct ggml_tensor * tensor) {
    return ggml_is_contiguous_n(tensor, 2);
}

@ggerganov
Copy link
Owner Author

Good catch - will revise tomorrow

@mofosyne mofosyne added the Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix label Jun 12, 2024
@ggerganov
Copy link
Owner Author

@slaren I implemented your suggestion. Should be ready to merge

Copy link
Collaborator

@slaren slaren left a comment

Choose a reason for hiding this comment

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

Looks good to me, I also verified that the assembly generated is very similar for both versions, so I don't expect this implementation to be slower. I only ran very limited tests though, and the logic is not very easy to follow for me, so I am not 100% sure that it will work in every case.

@ggerganov ggerganov merged commit bfaa676 into master Jun 12, 2024
66 of 73 checks passed
@ggerganov ggerganov mentioned this pull request Jun 14, 2024
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 Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants