ggml : add metal backend registry / device by ggerganov · Pull Request #9713 · ggml-org/llama.cpp

ggerganov · 2024-10-02T10:38:58Z

target #9707

Adapt the Metal backend to the new registry and device interfaces.

I have read the contributing guidelines
Self-reported review complexity:
- Low
- Medium
- High

slaren · 2024-10-02T13:02:57Z

ggml_backend_metal_buffer_type() also needs to be updated to set the device field.

slaren · 2024-10-03T00:01:37Z

There have been a few minor changes to the interfaces:

The get_backend_reg function of the device interface has been removed, instead a pointer is stored directly in ggml_backend_device: d0c4954
Some functions have been renamed: cfef355

mmtmn

lgtm

slaren · 2024-10-05T22:38:06Z

This seems to be working now.

ggml-ci

ggerganov · 2024-10-06T10:20:23Z

Should we put a deprecate notice for these API calls?

https://github.com/ggerganov/llama.cpp/blob/6dcb8991704b40d923691f037cdecc5430ff0440/ggml/include/ggml-metal.h#L41-L51

slaren · 2024-10-06T23:18:41Z


-ggml_backend_t ggml_backend_reg_metal_init(const char * params, void * user_data) {
+static const char * ggml_backend_metal_device_get_description(ggml_backend_dev_t dev) {
+    return [[g_state.mtl_device name] UTF8String];


I don't think there is a guarantee that mtl_device is initialized here, it probably needs a call to ggml_backend_metal_get_device/ggml_backend_metal_free_device like in ggml_backend_metal_device_get_memory. However, I imagine that could cause issues with the lifetime of the string returned by MTLDevice, so it may be necessary to keep a copy of the string in the context instead.

Should be fixed now.

I also reworked the implementation to avoid accessing g_state when we can get the device context locally. Should be much cleaner now and easier to add multi-GPU support in the future if needed.

ggml-ci

…-2-add-metal

slaren · 2024-10-07T13:45:59Z

 #if TARGET_OS_OSX || (TARGET_OS_IOS && __clang_major__ >= 15)
    if (@available(macOS 10.12, iOS 16.0, *)) {
-        GGML_LOG_INFO("%s: recommendedMaxWorkingSetSize  = %8.2f MB\n", __func__, ctx->device.recommendedMaxWorkingSetSize / 1e6);
+        GGML_LOG_INFO("%s: recommendedMaxWorkingSetSize  = %8.2f MB\n", __func__, device.recommendedMaxWorkingSetSize / 1e6);
    }
 #elif TARGET_OS_OSX
-    if (ctx->device.maxTransferRate != 0) {
-        GGML_LOG_INFO("%s: maxTransferRate               = %8.2f MB/s\n", __func__, ctx->device.maxTransferRate / 1e6);
+    if (device.maxTransferRate != 0) {
+        GGML_LOG_INFO("%s: maxTransferRate               = %8.2f MB/s\n", __func__, device.maxTransferRate / 1e6);


I don't think this #if/#elif is correct, this will never be printed.

slaren · 2024-10-07T14:28:36Z

Should we put a deprecate notice for these API calls?

I think it may be too early for that, it's probably better to wait a bit until all the backends and the ggml examples are updated.

* ggml : add metal backend registry / device ggml-ci * metal : fix names [no ci] * metal : global registry and device instances ggml-ci * cont : alternative initialization of global objects ggml-ci * llama : adapt to backend changes ggml-ci * fixes * metal : fix indent * metal : fix build when MTLGPUFamilyApple3 is not available ggml-ci * fix merge * metal : avoid unnecessary singleton accesses ggml-ci * metal : minor fix [no ci] * metal : g_state -> g_ggml_ctx_dev_main [no ci] * metal : avoid reference of device context in the backend context ggml-ci * metal : minor [no ci] * metal : fix maxTransferRate check * metal : remove transfer rate stuff --------- Co-authored-by: slaren <slarengh@gmail.com>

github-actions Bot added ggml changes relating to the ggml tensor library for machine learning Apple Metal https://en.wikipedia.org/wiki/Metal_(API) labels Oct 2, 2024

ggerganov changed the title ~~ggml-backend : add device and backend reg interfaces~~ ggml : add metal backend registry / devic Oct 2, 2024

ggerganov changed the title ~~ggml : add metal backend registry / devic~~ ggml : add metal backend registry / device Oct 2, 2024

ggerganov mentioned this pull request Oct 2, 2024

ggml-backend : add device and backend reg interfaces #9707

Merged

ggerganov force-pushed the sl/backend-registry-2-add-metal branch from 37de34c to a62ea59 Compare October 4, 2024 11:11

ggerganov changed the base branch from sl/backend-registry-2 to master October 4, 2024 11:11

ggerganov force-pushed the sl/backend-registry-2-add-metal branch 2 times, most recently from 058430f to ae56ec2 Compare October 4, 2024 12:15

mmtmn approved these changes Oct 4, 2024

View reviewed changes

ggerganov commented Oct 4, 2024

View reviewed changes

Comment thread ggml/src/ggml-backend.cpp

slaren force-pushed the sl/backend-registry-2-add-metal branch 2 times, most recently from 7e8d2a9 to 84c3b2a Compare October 5, 2024 22:47

ggerganov added 3 commits October 6, 2024 13:09

ggml : add metal backend registry / device

6214600

ggml-ci

metal : fix names [no ci]

2d8c2c7

metal : global registry and device instances

2e7e05c

ggml-ci

ggerganov added 2 commits October 6, 2024 13:10

metal : fix indent

4b161bc

metal : fix build when MTLGPUFamilyApple3 is not available

6dcb899

ggml-ci

ggerganov force-pushed the sl/backend-registry-2-add-metal branch from 84c3b2a to 6dcb899 Compare October 6, 2024 10:16

ggerganov marked this pull request as ready for review October 6, 2024 10:16

ggerganov requested a review from slaren October 6, 2024 10:17

fix merge

b150ffa

slaren reviewed Oct 6, 2024

View reviewed changes

ggerganov added 6 commits October 7, 2024 10:47

metal : avoid unnecessary singleton accesses

5f71096

ggml-ci

metal : minor fix [no ci]

1bd5018

metal : g_state -> g_ggml_ctx_dev_main [no ci]

34e0e6e

metal : avoid reference of device context in the backend context

70ff50d

ggml-ci

metal : minor [no ci]

2bd826d

Merge remote-tracking branch 'origin/master' into sl/backend-registry…

a70379d

…-2-add-metal

slaren approved these changes Oct 7, 2024

View reviewed changes

metal : fix maxTransferRate check

2294f07

metal : remove transfer rate stuff

901691c

ggerganov merged commit d5ac8cf into master Oct 7, 2024

ggerganov deleted the sl/backend-registry-2-add-metal branch October 7, 2024 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ggml : add metal backend registry / device#9713

ggml : add metal backend registry / device#9713
ggerganov merged 17 commits into
masterfrom
sl/backend-registry-2-add-metal

ggerganov commented Oct 2, 2024

Uh oh!

slaren commented Oct 2, 2024

Uh oh!

slaren commented Oct 3, 2024

Uh oh!

mmtmn left a comment

Uh oh!

Uh oh!

slaren commented Oct 5, 2024

Uh oh!

ggerganov commented Oct 6, 2024

Uh oh!

slaren Oct 6, 2024

Uh oh!

ggerganov Oct 7, 2024

Uh oh!

slaren Oct 7, 2024

Uh oh!

slaren commented Oct 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ggerganov commented Oct 2, 2024

Uh oh!

slaren commented Oct 2, 2024

Uh oh!

slaren commented Oct 3, 2024

Uh oh!

mmtmn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slaren commented Oct 5, 2024

Uh oh!

ggerganov commented Oct 6, 2024

Uh oh!

slaren Oct 6, 2024

Choose a reason for hiding this comment

Uh oh!

ggerganov Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

slaren Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

slaren commented Oct 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants