Skip to content

[SYCL][libdevice] Refactor cmake for imf SYCL device library #6312

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

Merged
merged 7 commits into from
Jun 18, 2022

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Jun 15, 2022

Signed-off-by: jinge90 [email protected]

This PR aims to refactor cmake building system for imf SYCL device library. Previously, the source of SYCL fallback device library has pre-assumption that all functions in the same fallback .spv or .o file must reside in single .cpp file. The reason is we need to use sycl compiler to build .spv or .o file with following commands:
clang++ -fsycl -fsycl-device-only -fno-sycl-use-bitcode t.cpp -o t.spv
clang++ -fsycl -c -fsycl-targets="..." t.cpp -o t.o

However, imf fallback sources broke the pre-assumptions, all implementations reside in multiple .cpp files. Current building have to deal with multiple tools, for spv building, we have to do following:

  1. use "clang++ -fsycl -fsycl-device-only -c -emit-llvm" to generate LLVM .bc file for each .cpp
  2. use "llvm-link" to link multiple LLVM .bc into one LLVM .bc file
  3. use "llvm-spirv" to convert the LLVM .bc to spirv file

For .o file, the steps are more complicated, we need to do following:

  1. use "clang++ -fsycl -fsycl-device-only -c -emit-llvm" to generate LLVM.bc for each .cpp
  2. Use "llvm-link" to link multiple LLVM .bc into one LLVM .bc file
  3. the steps 1,2 must be done for all sycl targets supported
  4. use clang-offload-bunlder to bundle different sycl target's LLVM .bc to final .objet file

Current solution has following drawbacks:

  1. Too complicated, too much unnecessary work which is to emulate the work of simple “clang++ -fsycl …” command. This makes cmake hard to understand and maintain.
  2. Too fragile, clang driver may change the detail steps of simple “clang++ -fsycl…” and each tool used currently may update their option and usage which may lead to imf libdevice building failure. The more tools we deal with, the more risk we are in.

So, we decide to use following solution:
For building imf fallback spv or object file, we first combine all required .cpp into a “full” .cpp and then use simple “clang++ -fsycl …” command to build it. We will create a “libdevice” folder in build/lib and save the full .cpp file there.
All imf fallback device library code will be organized to avoid code conflict, duplicate.

@jinge90 jinge90 requested review from a team as code owners June 15, 2022 14:53
@jinge90 jinge90 requested a review from aelovikov-intel June 15, 2022 14:53
@jinge90 jinge90 marked this pull request as draft June 15, 2022 14:53
@jinge90 jinge90 requested a review from zettai-reido June 16, 2022 07:57
@jinge90 jinge90 marked this pull request as ready for review June 16, 2022 08:00
@jinge90 jinge90 requested a review from xtian-github June 17, 2022 00:07
Copy link

@xtian-github xtian-github left a comment

Choose a reason for hiding this comment

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

LGTM

@aelovikov-intel aelovikov-intel requested a review from a team June 17, 2022 00:22
@aelovikov-intel
Copy link
Contributor

@intel/llvm-reviewers-runtime , who is fluent enough in this area to provide meaningful approval?

Copy link

@akolesov-nv akolesov-nv left a comment

Choose a reason for hiding this comment

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

Looks appropriate

@jinge90
Copy link
Contributor Author

jinge90 commented Jun 17, 2022

Hi, @pvchupin
Could you take a look of this PR?
Thanks very much.

Copy link

@zettai-reido zettai-reido left a comment

Choose a reason for hiding this comment

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

Cmake part looks good.

@pvchupin
Copy link
Contributor

@tcreech-intel, @zhaomaosu, could you please review?

Copy link
Contributor

@tcreech-intel tcreech-intel left a comment

Choose a reason for hiding this comment

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

I think this looks good.

Another approach might be to concatenate with the preprocessor, e.g., by introducing a top-level source which consists only of #include "imf_source.cpp" lines. This could either be generated by CMake from imf_f32_fallback_src_list and imf_fp64_fallback_src_list or checked into sources. In the latter case the CMake is greatly simplified. I don't necessarily prefer this, but thought it might be worth mentioning.

@pvchupin pvchupin merged commit 77bb972 into intel:sycl Jun 18, 2022
@xtian-github
Copy link

Thanks Pavel

@pvchupin
Copy link
Contributor

@jinge90, please address post-commit issues: https://github.com/intel/llvm/actions/runs/2518694462

pvchupin pushed a commit that referenced this pull request Jun 19, 2022
…device (#6324)

This patch aims to fix post-commit issue for #6312
Building libimf SYCL device library depends on sycl-compiler

Signed-off-by: jinge90 <[email protected]>
@jinge90 jinge90 deleted the simplify_cmake_for_imf_libdevice branch August 26, 2022 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants