ggml-threading.cpp #7576
Draft
ggml-threading.cpp #7576
+420
−364
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
build
mofosyne
added
the
Review Complexity : Medium
|
I'll need to update for #7598 |
slaren
reviewed
May 28, 2024
Comment on lines
+19
to
+22
| extern void atomic_store(atomic_int* ptr, LONG val); | ||
| extern LONG atomic_load(atomic_int* ptr); | ||
| extern LONG atomic_fetch_add(atomic_int* ptr, LONG inc); | ||
| extern LONG atomic_fetch_sub(atomic_int* ptr, LONG dec); |
There was a problem hiding this comment.
I think it is very important that these functions are inlined, so they cannot be made extern. Additionally, we cannot export symbols with these names, because they are very likely to create conflicts. Same with the pthread and sched_yield stuff.
There was a problem hiding this comment.
I see. Valid points.
That's the exact kind of feedback I was looking for. Thanks.
Comment on lines
+115
to
+116
| extern void set_numa_thread_affinity(int thread_n); | ||
| extern void clear_numa_thread_affinity(void); |
There was a problem hiding this comment.
Same issue here, we cannot export functions with these names. At least they need a ggml_ prefix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move all the threading related stuff to it's own file as discussed in the text of #6915
I picked cpp because I was thinking we'd use std::thread so we won't have to maintain two implementations of the thread operations. It'll also let us use std::mutex and std::atomic, and those primitives will likely be faster than our own implementations. However, this would just be the leading structural change. My goal with this PR is to not make any functional changes, and then we can do separate tests of the actual functional changes.
@slaren @ggerganov Do you think I'm headed in the right direction with the structural changes? I still need to do some more testing and self-reviews and I've only tested this really in windows so far, but I want to make sure I'm not headed in the wrong direction.