Skip to content

Connect support for dynamic linking to user options #14575

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
merged 26 commits into from
Aug 2, 2024
Merged

Connect support for dynamic linking to user options #14575

merged 26 commits into from
Aug 2, 2024

Conversation

LU-JOHN
Copy link
Contributor

@LU-JOHN LU-JOHN commented Jul 15, 2024 •
edited
Loading

Add option "-fsycl-allow-device-dependencies" to enable support for dynamic linking.

Also:

  1. No functions are importable without "-fsycl-allow-device-dependencies"
  2. Deal with SYCL_EXTERNAL header decls that have lost SYCL_EXTERNAL attribute in LLVM IR
  3. SPIRV/SYCL/ESIMD builtins cannot be an imported function

Tested in three E2E test cases.

Minor change:
Change SYCL-EXTERNAL to SYCL_EXTERNAL in testcase comment.

@LU-JOHN LU-JOHN requested review from a team as code owners July 15, 2024 20:17
sarnex
sarnex reviewed Jul 15, 2024
View reviewed changes
clang/include/clang/Driver/Options.td Outdated
Comment on lines 4191 to 4193
def fsycl_allow_split_with_dependencies : Flag<["-"], "fsycl-allow-split-with-dependencies">,
Group<sycl_Group>, HelpText<"Allow dependencies between device code images">;
def fno_sycl_allow_split_with_dependencies : Flag<["-"], "fno-sycl-allow-split-with-dependencies">,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we need approval from the options group for these, @mdtoguchi?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sarnex and @LU-JOHN, any new option (that we are inventing) that is meant to be visible should go through the options team workgroup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add new option in separate PR.

@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 15, 2024 21:05 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 15, 2024 21:53 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 16, 2024 00:34 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 16, 2024 01:17 — with GitHub Actions Inactive
sarnex
sarnex approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

lgtm only minor comments, the supportDynamicLinking function is a bit strange but i'm assuming this is an incremental change and it will be fleshed out soon, so it's fine with me

@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 16, 2024 19:18 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 16, 2024 21:12 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 17, 2024 13:35 — with GitHub Actions Inactive
@LU-JOHN
Copy link
Contributor Author

LU-JOHN commented Jul 17, 2024

lgtm only minor comments, the supportDynamicLinking function is a bit strange but i'm assuming this is an incremental change and it will be fleshed out soon, so it's fine with me

Yes, this function can only return true with the -shared option when the SYCL RT is ready. Otherwise, "-shared" tests may start failing.

@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 17, 2024 15:38 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 23, 2024 14:30 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 23, 2024 15:51 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN marked this pull request as draft July 24, 2024 03:38
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 24, 2024 04:00 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 24, 2024 04:47 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 24, 2024 13:04 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 24, 2024 14:04 — with GitHub Actions Inactive
Lu, John added 7 commits July 29, 2024 12:26
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 29, 2024 20:52 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 29, 2024 23:32 — with GitHub Actions Inactive
@sergey-semenov sergey-semenov mentioned this pull request Jul 30, 2024
Merged
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 30, 2024 15:39 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 30, 2024 20:51 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 30, 2024 23:53 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 31, 2024 14:53 — with GitHub Actions Inactive
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 31, 2024 16:11 — with GitHub Actions Inactive
@AlexeySachkov
Copy link
Contributor

All builds have successfully completed after merge with sycl branch. Considering that we had a fully green CI run on the second to last commit and that changes this PR introduces are into a feature that is not supported on CUDA and HIP, I think that we can merge this PR sooner without waiting for CUDA & HIP CI jobs to complete.

I will proceed with merge, but @LU-JOHN, @maarquitos14, please keep an eye on unfinished jobs in case they highlight any unexpected failures.

@AlexeySachkov AlexeySachkov merged commit 3c0532d into intel:sycl Aug 2, 2024
12 of 14 checks passed
@maarquitos14 maarquitos14 mentioned this pull request Aug 5, 2024
Merged
ldrumm pushed a commit that referenced this pull request Aug 7, 2024
@maarquitos14
[NFC] Clang-format fixes. (#14946)
65ee744 
Most likely the clang-format checker was down when
#14575 ran CI, so a few changes
non-clang-format-compliant made it in. Applying fixes to those.

---------

Signed-off-by: Marcos Maronas <[email protected]>
AlexeySachkov pushed a commit to AlexeySachkov/llvm that referenced this pull request Nov 26, 2024
@maarquitos14 @AlexeySachkov
Connect support for dynamic linking to user options (intel#14575)
2fe45f9 
Add option "-fsycl-allow-device-dependencies" to enable support for
dynamic linking.

Also:
1. No functions are importable without
"-fsycl-allow-device-dependencies"
2. Deal with SYCL_EXTERNAL header decls that have lost SYCL_EXTERNAL
attribute in LLVM IR
3. SPIRV/SYCL/ESIMD builtins cannot be an imported function

Tested in three E2E test cases.

Minor change:
Change SYCL-EXTERNAL to SYCL_EXTERNAL in testcase comment.

---------

Signed-off-by: Lu, John <[email protected]>
Co-authored-by: Marcos Maronas <[email protected]>
@AlexeySachkov AlexeySachkov mentioned this pull request Feb 26, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarnex sarnex sarnex approved these changes

@maarquitos14 maarquitos14 maarquitos14 approved these changes

@mdtoguchi mdtoguchi mdtoguchi 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.
5 participants
@LU-JOHN @AlexeySachkov @sarnex @maarquitos14 @mdtoguchi