Skip to content

[GPU] fix to embedding_bag_ref kernel - #37371

Draft
Roszczyk wants to merge 2 commits into
openvinotoolkit:masterfrom
Roszczyk:fix_embedding_beg_ref_kernel
Draft

[GPU] fix to embedding_bag_ref kernel#37371
Roszczyk wants to merge 2 commits into
openvinotoolkit:masterfrom
Roszczyk:fix_embedding_beg_ref_kernel

Conversation

@Roszczyk

@Roszczyk Roszczyk commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Details:

  • INPUT1_OFFSET was added twice. First, to start_indices and to end_indices, which are the range of the for loop and later to i in the same for loop. So finally indices_index was equal 2*INPUT1_OFFSET+offsets[offsets_ind]
  • in line 122 INPUT3_OFFSET was used to calculate weight_index while INPUT4_OFFSET represents the offset for weights. Setting INPUT3 is not required, and the check is set only to checking if INPUT4 is set, so it even might result in some crashes (INPUT3_OFFSET not declared).

Tickets:

AI

  • AI was used to speed up writing unit tests.

@github-actions github-actions Bot added the category: GPU OpenVINO GPU plugin label Aug 11, 2026
@Roszczyk
Roszczyk requested a lite review from Copilot August 12, 2026 11:53

Copilot AI 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.

Pull request overview

Fixes indexing/offset handling in the Intel GPU OpenCL reference kernel for embedding_bag, addressing incorrect double-application of INPUT1_OFFSET in offsets-based mode and incorrect offset selection for per-sample weights in segments-based mode.

Changes:

  • Fix OFFSETS_SUM start/end range computation to avoid adding INPUT1_OFFSET twice.
  • Fix SEGMENTS_SUM per-sample weights indexing to use INPUT4_OFFSET (weights input) instead of INPUT3_OFFSET.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI 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.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/plugins/intel_gpu/src/kernel_selector/cl_kernels/embedding_bag_ref.cl:122

  • [HIGH] Add a weighted segments_sum regression test where the weights tensor has a different physical offset from input 3. The current weighted test uses dense tensors (embedding_bag_gpu_test.cpp:776-777), making both offsets zero, so it cannot distinguish INPUT3_OFFSET from INPUT4_OFFSET and would not catch the original bug.
                uint weight_index = INPUT4_OFFSET + i;

src/plugins/intel_gpu/src/kernel_selector/cl_kernels/embedding_bag_ref.cl:56

  • [HIGH] Add a regression test that runs offsets_sum with a non-zero physical offset on the indices tensor. The existing GPU tests allocate dense, unpadded indices (for example, embedding_bag_gpu_test.cpp:376), so INPUT1_OFFSET is zero and they pass with both the old double-offset calculation and this fix. A padded/cropped indices input is needed to prove the corrected addressing and prevent this bug from returning.
    uint start_indices = offsets[offsets_ind];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: GPU OpenVINO GPU plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants