Skip to content

[SYCL] Implement coverage instrumentation for device code #20710

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.

Sign up for GitHub

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

Jump to bottom
Merged
steffenlarsen merged 9 commits into from
Dec 5, 2025
Merged

[SYCL] Implement coverage instrumentation for device code #20710

steffenlarsen merged 9 commits into from
Dec 5, 2025

Conversation

@0x12CC
Copy link
Contributor

@0x12CC 0x12CC commented Nov 20, 2025

This PR reapplies the changes from #20206. It also includes the following new changes:

  1. The coverage tools were moved from compiler dependencies to E2E test dependencies so that the SYCL toolchain can be be built without compiler-rt.
  2. A definition of __sycl_increment_profile_counters was added to fix build errors when building with Apple Clang.

0x12CC and others added 3 commits November 19, 2025 12:18
@0x12CC @steffenlarsen
[SYCL] Implement coverage instrumentation for device code (intel#20206)
4cf88f2 
This PR extends Clang's source-based code coverage to work with SYCL
device code. It includes the following changes:

1. The `InstrProfilingLoweringPass` was updated to lower profiling
counters to SYCL device globals.
2. A new function was added to the compiler runtime to increment the
host-side profiling counters.
3. The SYCL runtime was updated to send the contents of the device-side
profiling counters to the new compiler runtime function when their
device global map entries are being freed.

This feature may not work correctly for functions that differ between
host and device due to the use of the `__SYCL_DEVICE_ONLY__` macro. In
such cases, it may not be possible to correlate the profiling counters
from the device to the host. Resolves intel#7803.

---------

Signed-off-by: Michael Aziz <[email protected]>
Co-authored-by: Steffen Larsen <[email protected]>
Signed-off-by: Michael Aziz <[email protected]>
@0x12CC
Fix build error when compiling with Apple Clang
40f2af7 
Signed-off-by: Michael Aziz <[email protected]>
@0x12CC
Move profiling dependencies from compiler to E2E tests
3d8fee6 
Signed-off-by: Michael Aziz <[email protected]>
@0x12CC 0x12CC requested review from a team and bader as code owners November 20, 2025 15:41
@0x12CC
Copy link
Contributor Author

0x12CC commented Nov 20, 2025

@steffenlarsen, @AlexeySachkov, could you please help review this updated PR. The new changes added to fix the post-commit failures are in 40f2af7 and 3d8fee6.

mdtoguchi
mdtoguchi reviewed Nov 20, 2025
View reviewed changes
steffenlarsen
steffenlarsen approved these changes Nov 20, 2025
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Let's try throwing it at post-commit again and see what happens! 🚀

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Nov 20, 2025

Let's try throwing it at post-commit again and see what happens! 🚀

Push new branch to origin and trigger a manual post-commit run:
https://github.com/intel/llvm/actions/workflows/sycl-post-commit.yml
image

AlexeySachkov
AlexeySachkov approved these changes Nov 20, 2025
View reviewed changes
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

The two extra commits LGTM

srividya-sundaram
srividya-sundaram reviewed Nov 20, 2025
View reviewed changes
@0x12CC
Copy link
Contributor Author

0x12CC commented Nov 20, 2025

@aelovikov-intel, could you please run the post-commit testing? I don't think I have the required permissions to create a new branch or manually run any workflow.

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Nov 20, 2025

@aelovikov-intel, could you please run the post-commit testing? I don't think I have the required permissions to create a new branch or manually run any workflow.

https://github.com/intel/llvm/actions/runs/19546938418

Please ping me once that finishes to remove the branch.

@0x12CC
Copy link
Contributor Author

0x12CC commented Nov 20, 2025

Thanks, @aelovikov-intel. It's done so you can delete the branch.

The only test failure is in the Windows build:

Failed Tests (1):
  SYCL-Unit :: scheduler/./SchedulerTests_Non_Preview_Tests.exe/10/59

This is the same failure as in the post-commit testing for 2b96dd3 so it shouldn't be related to these changes.

@0x12CC 0x12CC requested a review from mdtoguchi November 25, 2025 21:35
@0x12CC 0x12CC requested a review from a team as a code owner November 28, 2025 03:43
@0x12CC
Copy link
Contributor Author

0x12CC commented Dec 2, 2025

@intel/dpcpp-cfe-reviewers, @bader, could you please provide reviews? I believe this PR is ready to merge.

bader
bader approved these changes Dec 2, 2025
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

compiler-rt/lib/profile/InstrProfilingRuntime.cpp looks okay.

@0x12CC
Copy link
Contributor Author

0x12CC commented Dec 3, 2025

@mdtoguchi, @srividya-sundaram, could you please provide a second review? I've addressed the comments but the PR still requires your approval.

srividya-sundaram
srividya-sundaram reviewed Dec 3, 2025
View reviewed changes
srividya-sundaram
srividya-sundaram approved these changes Dec 3, 2025
View reviewed changes
Copy link
Contributor

@srividya-sundaram srividya-sundaram left a comment

Choose a reason for hiding this comment

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

Just one question related to the test but otherwise, Driver changes LGTM!

@0x12CC
Add clang_cl test case
44e3a05 
Signed-off-by: Michael Aziz <[email protected]>
@0x12CC
Copy link
Contributor Author

0x12CC commented Dec 4, 2025

The test failure is the same as in #14613 and is unrelated to this change:

  ********************
  Timed Out Tests (1):
    SYCL :: HostInteropTask/host-task-failure.cpp

@intel/llvm-gatekeepers, this PR should be ready to merge.

@steffenlarsen steffenlarsen merged commit b1a75dd into intel:sycl Dec 5, 2025
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bader bader bader approved these changes

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@srividya-sundaram srividya-sundaram srividya-sundaram approved these changes

@elizabethandrews elizabethandrews elizabethandrews approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

@mdtoguchi mdtoguchi Awaiting requested review from mdtoguchi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@0x12CC @aelovikov-intel @bader @AlexeySachkov @srividya-sundaram @mdtoguchi @elizabethandrews @steffenlarsen