Skip to content

[SYCL][libdevice] Move common group utils from sanitizer specific header #18502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 19, 2025

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented May 16, 2025

We defined some group/subgroup util functions in device sanitizer specific headers, these functions will be used by other libdevice functions, so move them to a separate header.

@jinge90 jinge90 requested review from a team as code owners May 16, 2025 07:03
@jinge90 jinge90 requested a review from aelovikov-intel May 16, 2025 07:03
@jinge90 jinge90 requested review from Copilot and removed request for aelovikov-intel May 16, 2025 07:04
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors group utility functions in the libdevice library by moving common group utilities from the sanitizer-specific header into a dedicated header file.

  • Removed definitions of WorkGroupLinearId, SubGroupLinearId, and SubGroupBarrier from sanitizer_defs.hpp
  • Added a new header group_utils.hpp that contains the common group utility function definitions

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
libdevice/include/sanitizer_defs.hpp Removed group utility function definitions and added an include for group_utils.hpp
libdevice/include/group_utils.hpp New header containing group utility functions previously defined in sanitizer_defs.hpp
Files not reviewed (1)
  • libdevice/cmake/modules/SYCLLibdevice.cmake: Language not supported


#if defined(__SPIR__) || defined(__SPIRV__)

size_t WorkGroupLinearId() {
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

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

Consider marking these utility functions as 'inline' to avoid potential multiple definition issues if this header is included in multiple translation units.

Suggested change
size_t WorkGroupLinearId() {
inline size_t WorkGroupLinearId() {

Copilot uses AI. Check for mistakes.

@jinge90 jinge90 temporarily deployed to WindowsCILock May 16, 2025 07:10 — with GitHub Actions Inactive
@jinge90 jinge90 temporarily deployed to WindowsCILock May 16, 2025 07:32 — with GitHub Actions Inactive
@jinge90 jinge90 temporarily deployed to WindowsCILock May 16, 2025 07:32 — with GitHub Actions Inactive
Signed-off-by: jinge90 <[email protected]>
@jinge90
Copy link
Contributor Author

jinge90 commented May 17, 2025

Hi, @intel/llvm-gatekeepers
Could you help merge this PR?
Thanks!

@jinge90
Copy link
Contributor Author

jinge90 commented May 19, 2025

Hi, @intel/llvm-gatekeepers
Kind ping~

@aelovikov-intel aelovikov-intel merged commit 72fc869 into intel:sycl May 19, 2025
35 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants