Skip to content

ggml-webgpu: fix supports_op condition for GET_ROWS - #28978

Merged
yomaytk merged 2 commits into
ggml-org:masterfrom
yomaytk:fix-getrows-vec4-handling
Sep 18, 2026
Merged

yomaytk merged 2 commits into
ggml-org:masterfrom
yomaytk:fix-getrows-vec4-handling

Conversation

@yomaytk

@yomaytk yomaytk commented Sep 16, 2026

Copy link
Copy Markdown
Member

Overview

This PR removes the temporary supports_op logic for GET_ROWS in the WebGPU backend, which was needed for #28253 to pass the ops test.
PR #28382 adds the actual kernel change for that, so the temporary logic can now be removed. But vectorized alignment is not guaranteed by ggml_webgpu_tensor_align_offset, so this PR also adds an alignment check to the condition for the vectorized path.

The get_rows test coverage of test-backend-ops is not changed (207/207 passed)

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES, the code was written by Opus5 under my direction and reviewed by me.

@yomaytk
yomaytk requested a review from a team as a code owner September 16, 2026 03:41
@github-actions github-actions Bot added ggml changes relating to the ggml tensor library for machine learning WebGPU labels Sep 16, 2026
@yomaytk

yomaytk commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

@ServeurpersoCom Can you take a quick look at this when you have time? thanks!

@ServeurpersoCom ServeurpersoCom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the direction, the guard from #28253 is no longer needed after #28382 and falling back to the scalar path on GPU is better than CPU.

One remaining hole in the vectorized path: vec4_aligned only checks the offsets, but the shader also divides the src strides by 4, so a F32 view whose row stride is wider than ne0 still takes the vec4 path and reads wrong data. Checking the src strides in vec4_aligned as well fixes it, tested locally on Dawn. This predates the PR, the old guard only checked the offset too.

The existing vs0 tests only use whole row offsets on vec4 friendly shapes, so they never reach the non aligned path.

@yomaytk
yomaytk requested a review from ggerganov as a code owner September 18, 2026 04:47
@github-actions github-actions Bot added the testing Everything test related label Sep 18, 2026

@ggerganov ggerganov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test-backend-ops.cpp changes are OK

@yomaytk

yomaytk commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

@ServeurpersoCom
Sorry for the late reply, and good catch. I added the src stride checks to vec4_aligned in this PR.

I also added a new test_get_rows case with offset_cols = 3 in the vs0 path. It fails without the src stride checks but passes with them.

@yomaytk
yomaytk merged commit 44be98f into ggml-org:master Sep 18, 2026
26 of 30 checks passed
@yomaytk
yomaytk deleted the fix-getrows-vec4-handling branch September 18, 2026 11:47
@BrewTestBot BrewTestBot mentioned this pull request Sep 23, 2026
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ggml changes relating to the ggml tensor library for machine learning testing Everything test related WebGPU

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants