Skip to content

[SYCL]Use llvm-link's only-needed option to link device libs #2783

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 12 commits into from
Dec 10, 2020
Merged

[SYCL]Use llvm-link's only-needed option to link device libs #2783

merged 12 commits into from
Dec 10, 2020

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Nov 17, 2020

Signed-off-by: gejin ge.jin@intel.com

This PR aims to solve the same problem in a different way from #2723 which is a better way.
We will do 2-step link:

  1. normal llvm-link for all user LLVM bc to generate a "full" user.bc
  2. llvm-link -only-needed to link "full" user.bc with device libraries.
    In step, we expect only functions really required by user code will be linked in.
    We create the PR to run pre-ci tests and CPU performance tests.

@jinge90
Use llvm-link's only-needed option to link device libs
af48e23 
Signed-off-by: gejin <ge.jin@intel.com>
@jinge90
Fix clang-format issue
ee8a959 
Signed-off-by: gejin <ge.jin@intel.com>
@jinge90
Copy link
Contributor Author

jinge90 commented Nov 17, 2020

/summary:run

@jinge90 jinge90 requested a review from bader November 18, 2020 02:11
@jinge90
Copy link
Contributor Author

jinge90 commented Nov 18, 2020

Hi, @bader
This PR is based on your suggestion "option 2", we split the device link into 2 steps:

  1. normal llvm-link for all user's bc
  2. llvm-link only needed for devicelib bc.
    The code change is little, I ran full test suite and all passed. Also verified some perf benchmarks on CPU device, the perf improvement is as expected. Based on the IR dumped, no "double" functions were introduced into final device image if user code didn't include any double function, I will try on real platforms without double support later.
    I will have a look into your "option 1" later.
    Thanks very much.

@bader
Copy link
Contributor

bader commented Nov 18, 2020

@jinge90, thanks. Let me know the results of your investigation. I suggest closing #2723 - this solution is preferable in my opinion.

@jinge90
Copy link
Contributor Author

jinge90 commented Nov 18, 2020

@jinge90, thanks. Let me know the results of your investigation. I suggest closing #2723 - this solution is preferable in my opinion.

Yes, I will close #2723, no matter option 1 works or not, this PR is much better than #2723.
Thanks very much.

@jinge90 jinge90 mentioned this pull request Nov 18, 2020
Closed
mdtoguchi
mdtoguchi reviewed Nov 18, 2020
View reviewed changes
Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

Could a test be added to verify when -only-needed is added?

@jinge90
Copy link
Contributor Author

jinge90 commented Nov 19, 2020

Could a test be added to verify when -only-needed is added?

Hi, @mdtoguchi
We need some more investigation before making final decisions to optimize devicelib default. Once we decide to use this PR, I will add driver test to verify "-only-needed".
Thanks very much.

@jinge90 jinge90 mentioned this pull request Nov 30, 2020
Closed
@jinge90
Merge remote-tracking branch 'origin/sycl' into SyclDeviceLibsLinkOnl…
702d25e 
…yNeeded
@jinge90
Enable libm-fp64 default link
a3c7066 
Signed-off-by: gejin <ge.jin@intel.com>
@jinge90
Revert link all device libraries by default, plan to do this in a sep…
31133b6 
…arate patch together with all necessary driver option change

Signed-off-by: gejin <ge.jin@intel.com>
@jinge90
Fix clang-format issue
cffb9fe 
Signed-off-by: gejin <ge.jin@intel.com>
@jinge90
Fix sycl-device-lib lit failure
ebc68a8 
Signed-off-by: gejin <ge.jin@intel.com>
@jinge90
Fix sycl devicelib check issue on Windows
b4bce03 
Signed-off-by: gejin <ge.jin@intel.com>
@jinge90
Copy link
Contributor Author

jinge90 commented Dec 8, 2020

Could a test be added to verify when -only-needed is added?

Hi, @mdtoguchi
I have updated clang driver test sycl-device-lib and sycl-device-lib-win to add check for "llvm-link -only-needed" and I have updated both tests to adapt to latest compiler and device libraries(e.g. libsycl-glibc/msvc.o have been renamed to libsycl-crt.o).
Thanks very much.

@jinge90
Copy link
Contributor Author

jinge90 commented Dec 8, 2020

/summary:run

@jinge90
Copy link
Contributor Author

jinge90 commented Dec 9, 2020

Hi, @bader @vzakhari @mdtoguchi
Could you help to review this patch?
Thanks very much.

bader
bader reviewed Dec 9, 2020
View reviewed changes
@bader bader requested a review from mdtoguchi December 9, 2020 10:25
bader
bader reviewed Dec 9, 2020
View reviewed changes
@jinge90
Using std::find_if to replace for-loop when checking input SYCL devic…
37ffa4b 
…elibs

Signed-off-by: gejin <ge.jin@intel.com>
mdtoguchi
mdtoguchi approved these changes Dec 9, 2020
View reviewed changes
Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

LGTM

@bader
Copy link
Contributor

bader commented Dec 9, 2020

/summary:run

@bader bader merged commit e9423ff into intel:sycl Dec 10, 2020
sndmitriev
sndmitriev reviewed Dec 21, 2020
View reviewed changes
clang/lib/Driver/ToolChains/SYCL.cpp
if (InputFilename.startswith("libsycl-") &&
InputFilename.endswith(".o"))
return true;
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using file name for deciding if --only-needed option is required looks very fragile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @sndmitriev
I agree, we need some enhancement here .

alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Dec 21, 2020
Merge remote-tracking branch 'upstream/sycl' into sub_group_info_rework
4dba6e7 
* upstream/sycl: (616 commits)
  [SYCL][L0] Implement robust error handling in level_zero plugin (intel#2870)
  [SYCL][NFC] Code clean up (phase 5) revealed by self build. (intel#2907)
  [Driver][NFC] Remove unused variable (intel#2908)
  [Github Action] Enable automatic sync for main branch from llvm-project to llvm (intel#2904)
  [ESIMD][NFC] Remove unnecessary macro checks (intel#2900)
  [SYCL] Fix handling of multiple usages of composite spec constants (intel#2894)
  [SYCL] Adjust parallel-for range global size to improve group size selection (intel#2703)
  [SYCL] Add template parameter support for no_global_work_offset attribute (intel#2839)
  [SYCL] Support LLVM FP intrinsic in llvm-spirv and FE (intel#2880)
  [SYCL]Link Libm FP64 SYCL device library by default (intel#2892)
  [SYCL][NFC] Code clean up (phase 4) revealed by self build. (intel#2878)
  [SYCL][NFC] Code clean up (phase 3) revealed by self build. (intel#2865)
  [SYCL] Fix backend selection for SYCL_DEVICE_TYPE=* (intel#2890)
  [SYCL] Fix spec constants support in integration header (intel#2896)
  [Driver] Update unbundling of offload libraries to use archive type (intel#2883)
  [SYCL][NFC] Clang format SYCL.cpp (intel#2891)
  [CODEOWNERS] Add code owners for DPC++ tools (intel#2884)
  [XPTIFW] Enable in-tree builds (intel#2849)
  [SYCL] Don't dump IR and dot files by default in the LowerWGScope pass (intel#2887)
  [SYCL] Use llvm-link's only-needed option to link device libs (intel#2783)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader left review comments

@sndmitriev sndmitriev sndmitriev left review comments

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@AGindinson AGindinson Awaiting requested review from AGindinson

@vzakhari vzakhari Awaiting requested review from vzakhari

@andykaylor andykaylor Awaiting requested review from andykaylor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jinge90 @bader @mdtoguchi @sndmitriev