Repository navigation
Conversation
975d5b1 to
7d333cd
Compare
|
|
| *cur_backend_id = vsrc_backend_id; | ||
| GGML_ASSERT(vsrc_backend_id == -1 || ggml_backend_supports_op(sched->backends[*cur_backend_id], node)); | ||
| SET_CAUSE(node, "4.vsrc"); | ||
| } else if (!ggml_is_view_op(node->op) && !ggml_backend_sched_buffer_supported(sched, node->view_src, *cur_backend_id) && vsrc_backend_id != -1) { |
There was a problem hiding this comment.
This change doesn’t cover the case where the view_src has no backend yet (vsrc_backend_id == -1) because I am not sure such a case actually happen.
If you are aware of such a case, I would appreciate it if you could let me know.
cb85f71 to
e77f40a
Compare
| *cur_backend_id = vsrc_backend_id; | ||
| GGML_ASSERT(vsrc_backend_id == -1 || ggml_backend_supports_op(sched->backends[*cur_backend_id], node)); | ||
| SET_CAUSE(node, "4.vsrc"); | ||
| } else if (!ggml_is_view_op(node->op) && !ggml_backend_sched_buffer_supported(sched, node->view_src, *cur_backend_id) && vsrc_backend_id != -1) { |
There was a problem hiding this comment.
This branch can only ever be executed if *cur_backend_id != -1, meaning that a backend ID has already been assigned. The code then proceeds to re-assign the backend. I think that if at all possible we should be assigning backends exactly once per node and then not override that assignment later on. Did you check which of the previous passes does the assignment? Would it be viable to intervene there instead?
There was a problem hiding this comment.
Did you check which of the previous passes does the assignment?
No, I didn't when I opened this PR. So I checked which pass does the assignment on the WebGPU backend for the recent CI failure reported here, and found that pass 2 does it.
In general, the backend of node->view_src is not checked at all when a node is assigned before pass 4, so similar failures could also occur in pass 3.
I think that if at all possible we should be assigning backends exactly once per node
Would it be viable to intervene there instead?
Yes, I agree with that. So instead of overriding the assignment in pass 4, I changed passes 2 and 3 so that each node is assigned only once. Pass 2 assigns a backend to nodes only when node->view_src == NULL, and pass 3 considers only backends that can access the view_src buffer for nodes with node->view_src != NULL.
I propose this new change in the following commit instead, and I confirmed that test-llama-archs passes on the WebGPU backend on my M5. Could you take a look at this?
There was a problem hiding this comment.
Can you please force-push that here so that we have a record of how we decided to do things in a single place?
There was a problem hiding this comment.
Got it, I've force-pushed it.
There was a problem hiding this comment.
Hmm, this change caused a new CI failure in the Meta backend on nvidia: https://github.com/ggml-org/llama.cpp/actions/runs/37757525194/job/113245637763?pr=28075#step:9:131
I'm looking into it.
b1c3c46 to
31bfb36
Compare
Overview
This PR fixes a scheduler bug and enables the WebGPU backend to pass all the
currently skipped models in test-llama-archs (deepseek32, glm-dsa, dots3note,
qwen4exp).
When a node writes into a view of another tensor, it must be assigned to a
backend that can access the
view_srcbuffer, since the scheduler copies inputsacross backends but not outputs.
For the skipped models on the WebGPU backend, the following code in the DSA
variant of build_attn causes the error:
ggml_tensor * kq_mask_all = ggml_fill(ctx0, kq_mask, -INFINITY); … ggml_tensor * kq_mask_top_k = ggml_set_rows(ctx0, kq_mask_all, …);The WebGPU FILL op does not support the F16
kq_mask, so the FILL is assigned to the CPU but the SET_ROWS is assigned to WebGPU. The SET_ROWS output is a view of the FILL result, which causes the error.So this PR adds logic to reassign the writer node to a backend that can access
the view_src buffer.
Requirements