broken change: deprecate GGML_TASK_INIT and GGML_TASK_FINALIZE

[mqy]

Try resolve ggerganov/ggml#284

• comment warning for GGML_TASK_FINALIZE in ggml.h
• removed codes using GGML_TASK_FINALIZE in ggml.c, including those in ggml_graph_compute.
• print a warning message in ggml_compute_forward to warn unmerged incoming PR. Thread unsafe but should be OK. This message will be shown before prompt echo back in console.

Edit

I had missed a case that ggml_compute_forward_cross_entropy_loss_f32() is using GGML_TASK_FINALIZE.

Added a finalizer lookup table to patch this special case. Only one entry in it: ggml_compute_forward_cross_entropy_loss_finalizer. ggml_graph_compute_thread() was slightly updated.

Unable to test op GGML_OP_CROSS_ENTROPY_LOSS because the train always crash in the middle on my weak device. The extracted finalizer is quite simple, so I think it's OK to bypass the test by me.

Verified the lookup table with OP_MUL_MAT, the registered runner was called as expected.
Finally I removed GGML_TASK_FINALIZE.

Update

Also deprecated GGML_TASK_INIT; no longer touch runner codes, because that's error prone. We can cleanup them later step by step.

The GGML_OP_HAS_XXX idea is stolen from 9d058c2 Credit @zrm

@ggerganov @xaedes @zrm

[ggerganov]

This is great. Please rebase on latest master as there are recent changes to the threading logic due to #1556

[ggerganov]

Oops, I have missed that we are now using GGML_TASK_FINALIZE here.

@xaedes
I'm thinking about deprecating GGML_TASK_FINALIZE and this is the only exception that currently uses it. Do you think there is a way to make this work without the need for "finalize" pass?

[xaedes]

Hm... I don't think so.
There needs to be some finalizing synchronization for computing aggregated values.
Similarly ggml_compute_forward_sum_f32 (or mean) currently is not, but could be parallelized.
Then it also would need a final synchronization step.
The only way I see is to split it in multiple operations, storing intermediate tensors and then use a not-parallelizeable-by-design aggregating operation on the intermediate tensor.
For cross entropy this would create a huge intermediate tensor of size n_vocab * n_ctx * n_batch. It is pretty huge because n_vocab is 32001.

I think finalizer lookup table is a good idea to avoid the finalize stage for operations that don't need it.

[xaedes]

Atomic (add) operations could also be used to avoid finalizing synchronization, but such atomic functions often only work on integral types.

[mqy]

Atomic (add) operations could also be used to avoid finalizing synchronization, but such atomic functions often only work on integral types.

Current "problem" can be resolved by any kind of lock, but that's ugly and low efficient. Using atomic directly in compute code is a bad smell, that tends to result in difficult situations.

Let me share a post about linux spinlock, not quite relevant to current topic https://www.realworldtech.com/forum/?threadid=189711&curpostid=189723v

Quote:

... There's a reason why you can find decades of academic papers on locking. Really. It's hard.

Recently I tried c++ locks, feels bad, really. Any kind of logically correct locking has to be implemented by the underlying platform, but cross-platform sucks, CPU matters.

[ggerganov]

Yea - locks are not good.

@mqy
Can you try to adapt this solution proposed by @zrm in a recent PR:

9d058c2

We ended up not using it, but I think in the current situation it will be the best option.
The idea is to introduce a bool array that specifies which ops need to run GGML_TASK_FINALIZE.
It will effectively be full of false except for the cross entropy op.
The array has to be initialized in ggml_init() in is_first_call branch

If you do it, you can also add another array for GGML_TASK_INIT in a similar manner.
But we can leave this for later.

[mqy]

Sure, the bool array is good, no longer have to split finalizers, I think @ xaedes will feel happy :)
I'll do that, add the GGML_OP_HAS_INIT array as well.

[mqy]

Also deprecated GGML_TASK_INIT.
No longer touch runner codes, because that would modify or delete many lines, tedious and error prone.
We'd cleanup them later step by step.

@ggerganov pls have a look.

[mqy]

The macOS-make-latest build failed by "brew update failed"

[ggerganov]

Perfect!
I will merge this now.

I think we can improve the developer's experience by memsetting the op arrays to true so that all ops are enabled by default and then explicitly disable the ones that do not need INIT or FINALIZE.

This way I think we will avoid the situations where people wonder why no INIT or FINALIZE pass is being executed for their newly added ops. Let me know if you agree this is a good idea

[mqy]

Sorry for the delay, I'm back.

... then explicitly disable the ones that do not need INIT or FINALIZE.

MAY be better to keep the default behavior for some days, but I think it's OK to keep disabled by default, let me explain:

Suppose a developer committed initial implementation for a newly added OP X.
Somebody saw it does not need INIT, so disabled the INIT of OP X silently.
Some days later, the developer updated his/her algorithm that need INIT again, but does not aware of INIT was disabled.
In this situation, the developer is confused (even more).

Disabling INIT/FINALIZED by default is one of the new architecture requirement, developers SHOULD keep it in mind. This makes no difference to upgrading of an API spec. If we can not avoid breaking APIs, we can not avoid breaking architecture as well.

So, instead of keeping enabled by default, I prefer document warning at ggml_op (or some good place else), hopefully redirect developer to GGML_HAS_xxx.