Skip to content
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

[SYCL][NVPTX] Create one bitcode library for NVPTX #15048

Merged
merged 5 commits into from
Sep 2, 2024
Merged

[SYCL][NVPTX] Create one bitcode library for NVPTX #15048

merged 5 commits into from
Sep 2, 2024

Conversation

MartinWehking
Copy link
Contributor

@MartinWehking MartinWehking commented Aug 13, 2024 •
edited
Loading

Create one single bitcode library for NVPTX by compiling each libdev file into
bitcode first, linking these together and running opt on them.

Strip away metadata by reusing prepare_builtins from libclc.

Remove NVPTX bundles from the libdev object files and remove any unbundling
action spawned by the Clang driver for the SYCL toolchain when compiling for the
NVPTX backend.

Make the driver link against the single bitcode libraries for NVPTX for the SYCL
toolchain when device library linkage is not excluded.

Ensure that the clang tests check for the correctness of the new clang driver
actions and check if the driver still links the device code against the itt
device libraries when device library linkage has been excluded.

Refactor SYCLLibdevice.cmake by creating functions and grouping e.g. the same
compilation flags for a filetype together in one variable. Reuse these variables
and call functions to remove redundancies.

Create one bitcode library for NVPTX
3668c0a 
Create one single bitcode library for NVPTX by compiling each
libdev file into bitcode first, linking these together and running
opt on them.

Strip away metadata by reusing prepare_builtins from libclc.

Remove NVPTX bundles from the libdev object files and remove any
unbundling action spawned by the Clang driver for the SYCL toolchain
when compiling for the NVPTX backend.

Make the driver link against the single bitcode libraries for NVPTX
for the SYCL toolchain when device library linkage is not excluded.

Ensure that the clang tests check for the correctness of the new
clang driver actions and check if the driver still links the device
code against the itt device libraries when device library linkage has
been excluded.

Refactor SYCLLibdevice.cmake by creating functions and grouping
e.g. the same compilation flags for a filetype together in one variable.
Reuse these variables and call functions to remove redundancies.
@MartinWehking MartinWehking marked this pull request as ready for review August 13, 2024 15:08
@MartinWehking MartinWehking requested review from a team as code owners August 13, 2024 15:08
@bader
Copy link
Contributor

bader commented Aug 13, 2024

@MartinWehking, why this change is done only for NVPTX target? Can't we make the same change for other targets as well?

@MartinWehking
Copy link
Contributor Author

MartinWehking commented Aug 13, 2024 •
edited
Loading

@MartinWehking, why this change is done only for NVPTX target? Can't we make the same change for other targets as well?

Hi @bader
We also have a patch for the AMD target coming up (#15055).
Originally, there was just one patch for both targets (#13930).

It is probably possible to copy the work for the SPIRV target.
However, there might be unforeseen issues.

We encountered several issues with enabling it for AMD as well.
Therefore, this might be better to be done as future work.

@steffenlarsen
Copy link
Contributor

I would like @jinge90 to review this and maybe share some thoughts on the discussions above, especially pertaining to doing this for SPIR-V.

@MartinWehking MartinWehking mentioned this pull request Aug 19, 2024
Closed
@jinge90
Copy link
Contributor

jinge90 commented Aug 20, 2024 •
edited
Loading

I would like @jinge90 to review this and maybe share some thoughts on the discussions above, especially pertaining to doing this for SPIR-V.

The libdevice cmake change looks good to me.
For SPV target, these is no technical block to combine several fallback SPV file into one but I doubt whether it is correct direction. The combined SPV file will lead to more jit overhead for underlying runtime(IGC, OpenCL CPU RT), this may be a performance issue for small kernels.
For .obj/.o, these is no such problem since we use "llvm-link -only-needed" to clear away all unnecessary code in user's device image and then bundle the device code into final executable.
Thanks very much.

@MartinWehking
Copy link
Contributor Author

@jinge90

That sounds like a reasonable concern.
If there are no objections I would leave it as it is for this patch.

Fix subset devicelib linking for NVPTX
4223cfd 
Ensure that when subsets of devicelibs are excluded or included
by fno-sycl-device-lib or fsycl-device-lib, the correct libraries
are linked.
For -fsycl-device-lib this means that regardless of which libraries
are specified, the full single devicelib for NVPTX will be used.
For -fno-sycl-device-lib it means that its values will be ignored,
unless it contains "all", in which case only internal libraries
will be linked against.

Fix an error when printing a warning that some device libraries
have been ignored for -fno-sycl-device-lib
Add device lib linking test
e574b9a 
Check if device lib flags get treated correctly for NVPTX and
that the linking actions for the correct device libraries are generated.
Add more checks to NVPTX device-lib test
f914952 
Ensure that devicelib--cuda.bc is never linked against,
when -fno-sycl-devicle-lib contains the value "all".

Fix formatting of comments + run lines.
@npmiller
Copy link
Contributor

ping @intel/dpcpp-cfe-reviewers

@steffenlarsen given @jinge90 's input are you happy with this PR? It seems like it still makes sense for Nvidia and AMD but that we maybe shouldn't extend it to SPIR-V. But either way this also gives the CMake a much needed cleanup.

mdtoguchi
mdtoguchi approved these changes Aug 29, 2024
View reviewed changes
clang/lib/Driver/ToolChains/SYCL.cpp Outdated Show resolved Hide resolved
steffenlarsen
steffenlarsen approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Let's do it! 👍

@steffenlarsen steffenlarsen changed the title Create one bitcode library for NVPTX [SYCL][NVPTX] Create one bitcode library for NVPTX Aug 30, 2024
@mdtoguchi
Reformat clang/lib/Driver/ToolChains/SYCL.cpp
9a79dea 
Co-authored-by: Michael Toguchi <michael.d.toguchi@intel.com>
@MartinWehking
Copy link
Contributor Author

ping @intel/llvm-gatekeepers to get this merged

@ldrumm ldrumm merged commit 56a6ae2 into intel:sycl Sep 2, 2024
11 of 13 checks passed
@MartinWehking MartinWehking mentioned this pull request Sep 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steffenlarsen steffenlarsen steffenlarsen approved these changes

@elizabethandrews elizabethandrews elizabethandrews approved these changes

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@uditagarwal97 uditagarwal97 Awaiting requested review from uditagarwal97 uditagarwal97 is a code owner automatically assigned from intel/llvm-reviewers-runtime

@jinge90 jinge90 Awaiting requested review from jinge90

@hdelan hdelan Awaiting requested review from hdelan

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

Successfully merging this pull request may close these issues.
9 participants
@MartinWehking @bader @steffenlarsen @jinge90 @npmiller @hdelan @mdtoguchi @elizabethandrews @ldrumm