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

Implement scalar sum over all rows in ggml_compute_forward_sum_f32 #1162

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

xaedes
Copy link
Collaborator

@xaedes xaedes commented Apr 24, 2023

I was trying the test-grad0 test from ggml and this way I found a bug in ggml_compute_forward_sum_f32.
Only the sum of the last processed row was written to dst:

https://github.com/ggerganov/llama.cpp/blob/8a0f8673ba1cdc6aa6df27a9fbc698431ca70e8d/ggml.c#L3201-L3210

https://github.com/ggerganov/llama.cpp/blob/8a0f8673ba1cdc6aa6df27a9fbc698431ca70e8d/ggml.c#L6782-L6790

With this PR the sum over all rows is accumulated and then written to dst.

What is not so nice about it, is that the row sum is converted from ggml_float to float, losing precision during accumulation.


Was computing single sum over all rows like I did meant by this TODO?
https://github.com/ggerganov/llama.cpp/blob/8a0f8673ba1cdc6aa6df27a9fbc698431ca70e8d/ggml.h#L486-L489

only the sum of the last processed row was written to dst.
now the sum over all rows is written to dst.
Copy link
Member

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

Nice catch!

The confusion happened here: ggerganov/whisper.cpp#390
I didn't notice how ggml_vec_sum_f32() was being used in ggml_compute_forward_sum_f32() and made the mistake.

The TODO comment is very old and I have forgotten what it refers to, but for sure it is not related to this bug.
I think it might be related to adding an operator that computes the sums for each row and instead of returning a scalar, it returns [ne1, ne2, ne3, 1] tensor with the sums for each row of the input [ne0, ne1, ne2, ne3] tensor. But not 100% sure

@xaedes
Copy link
Collaborator Author

xaedes commented Apr 24, 2023

Ah okay, then we leave that TODO comment for later :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants