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

Connect support for dynamic linking to user options #14575

AlexeySachkov 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
Copy link
Contributor

@sarnex sarnex Jul 15, 2024

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

@mdtoguchi mdtoguchi Jul 15, 2024

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

@LU-JOHN LU-JOHN Jul 16, 2024

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
Test -fsycl-allow-device-dependencies with objects
3b8382b 
Signed-off-by: Lu, John <john.lu@intel.com>
Use preferred header. Ensure host code cannot be used.
f884ad0 
Signed-off-by: Lu, John <john.lu@intel.com>
-fPIC and -shared are Linux only. Device dependencies is not supporte…
7bd50b9 
…d for CUDA and hip

Signed-off-by: Lu, John <john.lu@intel.com>
Test -fsycl-allow-device-dependencies with a single DLL.
6e32667 
Signed-off-by: Lu, John <john.lu@intel.com>
Do not test on cuda or hip
21d4be4 
Signed-off-by: Lu, John <john.lu@intel.com>
Avoid failures on linux && gpu-intel-gen12 && opencl
f5fa46c 
Signed-off-by: Lu, John <john.lu@intel.com>
Avoid failures on windows && gpu-intel-gen12 && level_zero
cfaaa42 
Signed-off-by: Lu, John <john.lu@intel.com>
@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 added 2 commits July 30, 2024 08:26
Remove unsupported guards b/c of SYCL RT fix
2df4193 
Signed-off-by: Lu, John <john.lu@intel.com>
SYCL RT fix from Sergey Semenov
46a75e3 
Signed-off-by: Lu, John <john.lu@intel.com>
@LU-JOHN LU-JOHN temporarily deployed to WindowsCILock July 30, 2024 15:39 — with GitHub Actions Inactive
Restore Windows UNSUPPORTED guard
cf2b52b 
Signed-off-by: Lu, John <john.lu@intel.com>
@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
Fix Windows failure by adding dll suffix
2388a77 
Signed-off-by: Lu, John <john.lu@intel.com>
@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

AlexeySachkov commented Aug 2, 2024

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
@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 <marcos.maronas@intel.com>
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 <john.lu@intel.com>
Co-authored-by: Marcos Maronas <marcos.maronas@intel.com>
@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