Skip to content

[SYCL][NewOffloadModel] Merge linux/win tests for New Offloading Model. #20482

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
Open
wants to merge 9 commits into
base: sycl
Choose a base branch
from
Open

[SYCL][NewOffloadModel] Merge linux/win tests for New Offloading Model. #20482

wants to merge 9 commits into from
+86 −268

Conversation

@maksimsab
Copy link
Contributor

@maksimsab maksimsab commented Oct 27, 2025 •
edited
Loading

Initially, the sycl-linker-wrapper-win.cpp test was introduced to check .exe extensions. This change adapts the sycl-linker-wrapper.cpp test to any extensions.

Also, sycl-post-link-options-win.cpp test is removed because corresponding checks in the test are repeated in sycl-post-link-options.cpp test.

Fixes: #17754

@maksimsab
[SYCL][NewOffloadModel] Merge sycl-linker-wrapper-win.cpp test with
addd7dc 
sycl-linker-wrapper.cpp test

Initially, the sycl-linker-wrapper-win.cpp test was introduced to check .exe
extensions. This change adapts the sycl-linker-wrapper.cpp test to any
extensions.
@maksimsab maksimsab requested a review from a team October 27, 2025 15:37
@maksimsab maksimsab requested a review from a team as a code owner October 27, 2025 15:37
@maksimsab
Copy link
Contributor Author

Note: Not ready for review. I posted this PR to run the CI for Windows.

@maksimsab
Copy link
Contributor Author

Hi @intel/dpcpp-tools-reviewers @intel/dpcpp-clang-driver-reviewers
This PR is ready for review. I have also just noticed that we have 2 tests that are split to linux and windows versions - clang/test/Driver/sycl-post-link-options.cpp and clang/test/Driver/sycl-post-link-options-win.cpp.

I think I would like to merge them as well.

Also my note about merging tests is that it makes more difficult to check path separators if you want to check them.

@maksimsab maksimsab changed the title [SYCL][NewOffloadModel] Merge sycl-linker-wrapper-win.cpp test with sycl-linker-wrapper.cpp test [SYCL][NewOffloadModel] Merge linux/win tests for New Offloading Model. Oct 30, 2025
@maksimsab
Copy link
Contributor Author

@bader Please, share your feedback if you have any.

bader
bader approved these changes Oct 30, 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.

LGTM. 👍
Thanks!

@srividya-sundaram
Copy link
Contributor

I think I would like to merge them as well.

I would also suggest to rename the test file to clang-linker-wrapper.cpp , as the test is testing clang-linker-wrapper tool.

YuriPlyakhin
YuriPlyakhin approved these changes Oct 30, 2025
View reviewed changes
Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a comment

Choose a reason for hiding this comment

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

Renaming test sycl-linker-wrapper to clang-linker-wrapper also makes sense to me, but it was not the purpose of this PR, so I approve. Thanks.

@srividya-sundaram
Copy link
Contributor

Thank you @bader

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

@YuriPlyakhin YuriPlyakhin YuriPlyakhin approved these changes

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove code duplication in clang-linker-wrapper test.

4 participants

@maksimsab @srividya-sundaram @bader @YuriPlyakhin