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

Fix potential int8_t overflow in non-SIMD vec_dot #986

Merged
merged 1 commit into from
Apr 15, 2023
Merged

Conversation

sw
Copy link
Contributor

@sw sw commented Apr 14, 2023

As rightly pointed out by @jxy here, my changes in #703 limiting the calculation to int8_t might overflow.

-> Change the types to int instead.

@jxy
Copy link
Contributor

jxy commented Apr 14, 2023

You could also just do

diff --git a/ggml.c b/ggml.c
index 42e3ee3..80d539f 100644
--- a/ggml.c
+++ b/ggml.c
@@ -2216,7 +2216,10 @@ static void ggml_vec_dot_q4_0(const int n, float * restrict s, const void * rest
             const int8_t i2 = (int8_t) (v1 & 0xf) - 8;
             const int8_t i3 = (int8_t) (v1 >> 4)  - 8;
 
-            sumi += i0*i2 + i1*i3;
+            const int i02 = i0*i2;
+            const int i13 = i1*i3;
+
+            sumi += i02 + i13;
         }
         sumf += d0 * d1 * sumi;
     }

and keep those int8 multiplications.

@sw
Copy link
Contributor Author

sw commented Apr 15, 2023

keep those int8 multiplications.

Is there any processor people might reasonable use where that makes a difference? I'll change it if yes, otherwise it's more concise as it is.

@sw sw merged commit 2f7c8e0 into ggml-org:master Apr 15, 2023
@sw sw deleted the fix-int8 branch April 15, 2023 18:29
Deadsg pushed a commit to Deadsg/llama.cpp that referenced this pull request Dec 19, 2023
jeroen-mostert pushed a commit to jeroen-mostert/llama.cpp that referenced this pull request Aug 30, 2024
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.

3 participants