Skip to content

ggml : deprecate GGML_TASK_FINALIZE #284

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.

Sign up for GitHub

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

Jump to bottom
Closed
ggerganov opened this issue Jun 25, 2023 · 2 comments · Fixed by ggml-org/llama.cpp#1995
Closed

ggml : deprecate GGML_TASK_FINALIZE #284

ggerganov opened this issue Jun 25, 2023 · 2 comments · Fixed by ggml-org/llama.cpp#1995
Labels
good first issue Good for newcomers performance Speed related topics refactoring Refactoring

Comments

@ggerganov
Copy link
Member

The initial design of the compute tasks in ggml was each one to have 3 separate stages:

  • GGML_TASK_INIT
  • GGML_TASK_COMPUTE
  • GGML_TASK_FINALIZE

So far, the GGML_TASK_FINALIZE step has been left completely unused and it seems it won't find any applications in the future. Therefore, it is best to remove it all together. This will simplify the code a little bit and also bring performance improvements as there will be fewer thread synchronization points during the graph computation

Related:
@ggerganov ggerganov added enhancement New feature or request good first issue Good for newcomers labels Jun 25, 2023
@ggerganov ggerganov added refactoring Refactoring performance Speed related topics and removed enhancement New feature or request labels Jun 25, 2023
@ggerganov ggerganov moved this to Todo in ggml : roadmap Jun 25, 2023
@ggerganov ggerganov changed the title ggml : deprecate GGML_TASK_FINALIZE ggml : deprecate GGML_TASK_FINALIZE Jun 25, 2023
goerch added a commit to goerch/ggml that referenced this issue Jun 25, 2023
@goerch
Fix ggml-org#284
c6e28e8
@mqy mqy mentioned this issue Jun 25, 2023
Merged
goerch added a commit to goerch/ggml that referenced this issue Jun 26, 2023
@goerch
Merge remote-tracking branch 'upstream/master' into fix-ggml-org#284
6da8307
@ggerganov ggerganov moved this from Todo to In Progress in ggml : roadmap Jun 27, 2023
@ggerganov ggerganov moved this from In Progress to Done in ggml : roadmap Jul 1, 2023
@rlanday
Copy link

rlanday commented Sep 7, 2023

Hi, I took a look at this and found that GGML_TASK_FINALIZE is used by ggml_compute_forward_cross_entropy_loss_f32():

https://github.com/ggerganov/llama.cpp/blob/178b1850ebd21b349cebbee887950e435c5aa2d3/ggml.c#L15453

Please advise.

@ggerganov
Copy link
Member Author

INIT and FINALIZE are now optional and disabled for the majority of ops:

https://github.com/ggerganov/ggml/blob/79ea7a41a388e9e7c85e426e912ab38399c44bfa/src/ggml.c#L4080-L4116

This is good enough for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers performance Speed related topics refactoring Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

broken change: deprecate GGML_TASK_INIT and GGML_TASK_FINALIZE mqy/llama.cpp
2 participants
@ggerganov @rlanday