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

Adding unary and binary map operations #874

Merged
merged 3 commits into from
Apr 14, 2023

Conversation

KerfuffleV2
Copy link
Collaborator

Proof of concept support for mapping arbitrary unary/binary operations with GGML.

This would require support models that require operations that aren't currently implemented, even if it was in a basic/less than optimally performant way.

I was a C developer in a past life, but it's been a long time. This does appear to work (using map functions from Rust even) but it may not be safe.

While this most likely isn't in a state to be merged, is it something that could in theory get included? I'm willing to work with the powers that be. (Also, wasn't sure exactly where to submit this. It seems like the main ggml is significantly different from the llama.cpp one.)

ggml.c Outdated
case GGML_OP_ADD:
case GGML_OP_MAP_UNARY:
{
void (*const fun)(int, float *, float *) = *((void **)tensor->opt[0]->data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe name this type something like ggml_unary_op_t? (and then name the parameters in that type so that it’s clear what they mean)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely. At this point, I'm just trying to see if the maintainers would be open to the general idea of adding this type of operation.

The actual implementation can and should be cleaned up before it becomes a real pull. (This is POC is also missing the handling in backward building thing and setting the number of tasks.)

@KerfuffleV2
Copy link
Collaborator Author

This implementation is limited to float values only and unary/binary ops, but I think it wouldn't be too hard to make it generic. The backend part of adding the operation could just take an opaque point for the function pointer as well as a list of the types of the function pointer's arguments.

Then later on the actual processing could ensure that the operation is getting applied to the correct number of tensors with the expected types. It would be more complicated to do that, but it would be more flexible and only require one new op.

@ggerganov
Copy link
Owner

This is an interesting concept that we should definitely consider.
As @KerfuffleV2, I think we should aim for adding capability to implement generic operators, not just map.
Maybe take a shot at it in a separate PR and we can discuss both approaches and see how to improve the interface.

@KerfuffleV2
Copy link
Collaborator Author

@ggerganov

As @KerfuffleV2, I think we should aim for adding capability to implement generic operators, not just map.

I'm not quite sure what you mean. Implementing map operations was my path to being able to use generic operations (operators?). For example, you can see I use the map ops to add these functions: https://github.com/KerfuffleV2/smolrsrwkv/blob/a8fca25b978f39c31383940da9e6cdc21736d7bc/smolrwkv/src/ggml/map_ops.rs#L195

Which were required to implement RWKV: https://github.com/KerfuffleV2/smolrsrwkv/blob/experiment-ggml-model/smolrwkv/src/ggml/graph.rs

I've also done some performance testing and those operations basically don't even show up in the profiling metrics because basically it's just matmul that matters. I think they came to about 0.1%, so even a pretty simple/naive approach can get one to the point of a decent implementation for something that's not currently possible with GGML.

Anyway, I don't currently understand what you're asking me to change but I'm open to changing stuff. Also would be happy to update this to merge cleanly if it's something that seems like it might actually get merged. Right now, I'm in the position of looking at needing to maintain a separate version of GGML for my project.

Please keep in mind though, I'm not really a C/C++ developer so doing something really extensive may be beyond my abilities.

I guess one thing that could be added pretty easily is a 3d version + maybe fold operations? Managing the types for folding, especially across languages could be a pain though.

@ggerganov
Copy link
Owner

@KerfuffleV2
I was rushing through the PRs and now I realise that my comment does not make any sense - apologies.

The proposal and implementation is great. I don't think a 3D version is needed, atm.

Also would be happy to update this to merge cleanly if it's something that seems like it might actually get merged. Right now, I'm in the position of looking at needing to maintain a separate version of GGML for my project.

Yes please. The suggested typedef by @j-f1 is a good idea. Anything else that you think makes sense to improve it is welcome. After we merge this, I will backport it to the ggml repository and there we can add a test or an example to demonstrate usage.

@KerfuffleV2
Copy link
Collaborator Author

@ggerganov No need to apologize! I'm glad you like the general idea. Do you have any preference for where the code is in the file (I put the new operations in a semi-random place). Also, I'd just set the number of threads to 1 unless you think there's a reason to use something different.

At least with the operations I used for RWKV, it didn't seem like there was any benefit to using multiple threads when mapping.

@ggerganov
Copy link
Owner

Yes, single thread is OK for now.
Regarding the order - move them after ggml_flash_ff(). Both in ggml.h and ggml.c.
The idea is to follow the order of the enum ggml_op

Add handling for task setting.

Add handling for ggml_compute_backward.

Rename functions to ggml_map_unary_f32 and ggml_map_binary_f32

Fix compiler warnings related to casting function pointers and `void *`

Reorder functions and definitions based on the GGML op number.

Use typedefs for map op function pointer types.
@KerfuffleV2 KerfuffleV2 marked this pull request as ready for review April 14, 2023 10:10
@KerfuffleV2
Copy link
Collaborator Author

Changes relative to the previous version:

  1. Add handling for task setting.
  2. Add handling for ggml_compute_backward.
  3. Rename functions to ggml_map_unary_f32 and ggml_map_binary_f32
  4. Fix compiler warnings related to casting function pointers and void *
  5. Reorder functions and definitions based on the GGML op number.
  6. Use typedefs for map op function pointer types.
  7. Tagged arguments as const that wouldn't need to be mutable.

Hopefully renaming the functions isn't too controversial. My thinking is that it would be really hard to support addition types in the future with the previous naming convention. If you don't like this, I can change it back easily. Or if you prefer a different approach to naming based on the types, that's no problem either.

I've tested the updated GGML + these changes against my RWKV implementation and it appears to work without issues. (Only tested on Linux x86-64.)

@KerfuffleV2 KerfuffleV2 changed the title GGML map ops proof of concept. Adding unary and binary map operations Apr 14, 2023
@howard0su
Copy link
Collaborator

Love to see your change of RWKV.

ggml.c Outdated Show resolved Hide resolved
ggml.c Show resolved Hide resolved
}

struct ggml_tensor * addr_tensor = ggml_new_tensor_1d(ctx, GGML_TYPE_I32, sizeof(void *) / sizeof(int32_t));
*((void (**)(void))addr_tensor->data) = (void (*)(void))fun;
Copy link
Collaborator

Choose a reason for hiding this comment

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

will addr_tensor->data = (void*)fun works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"Works", yes, but it produces warnings when compiling like:

ggml.c:9085:50: warning: ISO C forbids initialization between function pointer and ‘void *’ [-Wpedantic]

ggml.c Show resolved Hide resolved
ggml.c Show resolved Hide resolved
@ggerganov ggerganov merged commit c9a59b7 into ggerganov:master Apr 14, 2023
@ggerganov
Copy link
Owner

@KerfuffleV2

Thanks for the contribution.
If you are interested, please consider adding examples / tests in https://github.com/ggerganov/ggml that demonstrate the usage of these new operators

@KerfuffleV2
Copy link
Collaborator Author

@ggerganov Perfect!

As for examples/tests, I'm not really that comfortable writing C. It looks like the examples there actually real applications. I'm also not sure about writing a test in C since my use case is actually from Rust.

What I could do is write some documentation with code fragments. Would that be useful? I'm not quite sure where to put it in that project though. Make an examples directory that just as a .md file in it maybe? Or make a new directory for docs?

If you were more specific about what you're looking for, I might be able to make an actual example or test but I think it probably would be better to have an actual C person do it.

@KerfuffleV2 KerfuffleV2 deleted the experiment-ggml-map-ops branch May 28, 2023 08:45
Deadsg pushed a commit to Deadsg/llama.cpp that referenced this pull request Dec 19, 2023
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.

4 participants