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

ggml : reduce the values of ggml_op enum #285

Closed
ggerganov opened this issue Jun 25, 2023 · 3 comments · Fixed by #405
Closed

ggml : reduce the values of ggml_op enum #285

ggerganov opened this issue Jun 25, 2023 · 3 comments · Fixed by #405
Labels
good first issue Good for newcomers refactoring Refactoring

Comments

@ggerganov
Copy link
Owner

ggerganov commented Jun 25, 2023

Here is another idea to consider for reducing the amount of unary ops and code duplication in ggml.c:

The following ops:

    enum ggml_op {
        ...
        GGML_OP_ABS,
        GGML_OP_SGN,
        GGML_OP_NEG,
        GGML_OP_STEP,
        GGML_OP_TANH,
        GGML_OP_ELU,
        GGML_OP_RELU,
        GGML_OP_GELU,
        GGML_OP_GELU_QUICK,
        GGML_OP_SILU,
        ...
    };

Can all become:

    enum ggml_unary_op {
        GGML_UNARY_OP_ABS,
        GGML_UNARY_OP_SGN,
        GGML_UNARY_OP_NEG,
        GGML_UNARY_OP_STEP,
        GGML_UNARY_OP_TANH,
        GGML_UNARY_OP_ELU,
        GGML_UNARY_OP_RELU,
        GGML_UNARY_OP_GELU,
        GGML_UNARY_OP_GELU_QUICK,
        GGML_UNARY_OP_SILU,    
    };

    GGML_API struct ggml_tensor * ggml_unary(
            struct ggml_context * ctx,
             struct ggml_tensor * a,
             enum ggml_unary_op   op);

Original task (completed via #313)

Certain ggml operators can be combined into a singe operator with extra arguments.

For example:

  • GGML_OP_CONV_1D_S1_PH
  • GGML_OP_CONV_1D_S2_PH

can all become:

  • GGML_OP_CONV_1D

with a signature of:

    GGML_API struct ggml_tensor * ggml_conv_1d(
            struct ggml_context * ctx,
            struct ggml_tensor  * a,
            struct ggml_tensor  * b
                            int   stride,
                            int   padding);

Internally, we can have custom implementations of ggml_compute_forward_conv_1d() for specific values of the parameters and GGML_ASSERT for everything else that is not supported.

This will allow to extend the functionality of these operator more easily in the future

@ggerganov ggerganov added good first issue Good for newcomers refactoring Refactoring labels Jun 25, 2023
@ggerganov ggerganov changed the title ggml : reduce ggml_op enum ggml : reduce the values of ggml_op enum Jun 25, 2023
@ggerganov
Copy link
Owner Author

bump (description updated)

@rlanday
Copy link

rlanday commented Sep 7, 2023

@ggerganov, it appears you did this already in 5b2b2dc. I recommend closing this issue.

@rlanday
Copy link

rlanday commented Sep 7, 2023

Oh never mind, I am looking at issues that are already completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers refactoring Refactoring
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants