-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL] Don't add itt libdevice into linking list for NV,AMD,NativeCPU backend #19603
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
base: sycl
Are you sure you want to change the base?
Conversation
Signed-off-by: jinge90 <[email protected]>
Signed-off-by: jinge90 <[email protected]>
Signed-off-by: jinge90 <[email protected]>
Hi, @hvdijk |
I'm on holiday, @intel/dpcpp-nativecpu-reviewers would someone else have a look at this PR? |
Hi, @intel/dpcpp-nativecpu-reviewers |
Hi, @intel/dpcpp-nativecpu-reviewers |
Hi, @mdtoguchi |
Hi, @intel/dpcpp-nativecpu-reviewers |
Hi, @npmiller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CUDA changes LGTM
@@ -5903,13 +5903,6 @@ class OffloadingActionBuilder final { | |||
SmallString<128> LibName(LLCandidate); | |||
llvm::sys::path::append(LibName, DeviceLib); | |||
if (llvm::sys::fs::exists(LibName)) { | |||
// NativeCPU currently only needs libsycl-nativecpu_utils and | |||
// libclc, so temporarily skip other device libs in invocation. | |||
// Todo: remove once NativeCPU tests the other libraries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the itt device library should be skipped for NativeCPU, wouldn't keeping this code achieve that, i.e. do you really need to remove this?
Note (as per the original comment) that removing this code will potentially add more device libraries to the NativeCPU link stage, which would slow down linking because NativeCPU currently only needs to link libclc
and libsycl-nativecpu_utils
. Linking more device libraries in NativeCPU would also need testing for the code linked from these libraries, which seems to go beyond what this PR intends to achieve, which is skipping the itt device lib for the three backends?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @uwedolinsky
Previously, itt device libraries were added to link list together with libsycl-native_utils for nativecpu target, so the logic here is needed to filter out all other libraries except libsycl-native_utils.
Instead of adding unneeded library and then add handling logic to filter out them for nativecpu target, we stops adding itt device libraries into link list when current target is nativecpu, so we don't need to filter out any unneeded libraries.
The PR description seems to be confusing, we just skip "adding" itt device libraries into link list for native cpu/cuda/amd target.
Thanks very much.
ITT device library is used by sycl-instrument-device-code for Intel Vtune support, they are not required by NV, AMD and NativeCPU backend. This PR removes ITT device library files into device linking list for these backend. After doing this, we don't need to add special check for NativeCPU backend in driver code to filter all libraries other than "native_cpu-utils".