ggml-cuda: reorder only relevant nodes

[am17an]

GGML_CUDA_GRAPH_OPT=1 is broken with any tensor offloading options like n-cpu-moe because we just copy the graph back to enable fusion within streams. This PR only re-orders nodes within streams.

Also, because we don't use CUDA graphs for hybrid inference at the moment, GGML_CUDA_GRAPH_OPT=1 is slower than not using it. This may change in the future

[wishstudio]

I'm trying to merging master on my PR #16548 and hit hard by this 😭

Is it possible to avoid changing cgraph in evaluate_and_capture_cuda_graph? Because the graph plan api generally expects to use const struct ggml_cgraph and does not expect the backend to change cgraph halfway (which is good design IMO).

Does the node changes need to be persistent after returning from this function? If not, isn't it easier to just create a temporary cgraph to use inside this function, and discard it before returning. struct ggml_cgraph is just a few pointers, duplicating won't affect performance at all.

It is of course possible to hack over this in my PR, but I feel the current solution is already a bit hacky and I would like to avoid a hack over a hack.

[am17an]

@wishstudio for now you should just not use this path in your PR (it is not enabled by default)

[am17an]

BTW @wishstudio do you still face an issue after this PR? I think it solves your particular issue

[wishstudio]

I’m not sure if my graph handling code is semantically correct with the nodes changing halfway but if you are still working on it it’s ok for me to skip this codepath rn. Without this PR it won’t even compile because the cgraph itself is being changed. I think this PR should make it work.

[JohannesGaessler]

@wishstudio sorry, I was not aware of your efforts and that the recently merged approach would be causing problems for you. As I'm sure you're aware Diego is currently on Hiatus. For this reason pull requests such as yours that have to do with his work will probably not receive good attention from maintainers. As it is I'm already operating at full capacity but I intend to also do work having to do with ggml backends. When I do I'll take a look at the ggml graph plan API and see if I can review your work (definitely no promises though). FWIW I don't consider the current implementation for concurrent CUDA streams to be something that we should be using long-term and I agree that ggml backends should not be changing ggml graphs if at all possible.

[am17an]

@wishstudio I'll take a look at your PR and try to review it as well

[wishstudio]

@am17an After you merged this PR I updated mine and it is working fine now. Thanks!

@JohannesGaessler Thank you for the explanation and I appreciate your stance on code quality. Please take your time! I think that PR also enables more graph based optimizations to be done in all the backends, but it still needs some design work. The annoying part atm is I did some refactoring and the diff all got messed up, causing conflicts every time evaluate_and_capture_cuda_graph is changed.

[am17an]

BTW to be clear as to there's no confusion. The nodes don't change "halfway through", they are captured by cuda graph and they remain the same way throughout until the graph is captured again.

This PR fixed a bug when the size of the cgraph changes because there are splits added for cpu moe, I will add code to disable this when cuda graphs cannot be used as it leads to a performance drop anyway.

Reordering or skipping nodes is allowed in graph compute under the correct conditions. The earlier PR plus this PR does not break this paradigm

[wishstudio]

BTW to be clear as to there's no confusion. The nodes don't change "halfway through", they are captured by cuda graph and they remain the same way throughout until the graph is captured again.

Yes I do understand that.

Reordering or skipping nodes is allowed in graph compute under the correct conditions.

This is exactly what I'm arguing. Since you added the graph optimize function, I would think all graph modifications done by a backend should be moved there. In my intuition an "optimize" operation is obviously allowed to change the thing it optimizes, but a "compute" operation is like calling a function, which in most cases one would not expect that it changes the function itself.

Of course in this case we can argue that it's more like a mutable cache inside an immutable function so the changes are invisible to the user. But based on the work on my PR I don't remember seeing any code doing actual graph changes in the CUDA backend (correct me if I'm wrong). The only similar thing is perhaps fusing but technically speaking it is done on the fly and the changes are never written to the graph. So I don't think this is strictly necessary.

For reordering and skipping nodes it's analogous to a superscalar CPU converting instructions to uops, optimizing them in a uops buffer and executing them in any order it likes. I'm perfectly ok with it as long as it uses separate buffers and the original instructions memory are kept intact.

[am17an]

Since you added the graph optimize function, I would think all graph modifications done by a backend should be moved there.

Yes I would prefer that as well. If you read the original PR there is a path forward for getting proper support for these kind of out-of-order operations, so eventually we would not require this hack.

That being said, currently even fusion modifies the cgraph in a few ways, not the memory buffer but the ggml_tensors, which also is a bad pattern that I plan to imminently remove.