Skip to content

[SYCL]Add device libraries for C math and complex #975

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 1 commit into from
Feb 11, 2020
Merged

[SYCL]Add device libraries for C math and complex #975

merged 1 commit into from
Feb 11, 2020

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Dec 26, 2019 •
edited by bader
Loading

Add device libraries for C math and complex functions based on SYCL compiler's device library infrastructure. Fallback implementation for math and complex functions are also provided. The fallback implementation for C math functions are based on _spirv_ocl* math built-ins and the fallback implementation for C99 complex functions are based on algorithm used in libc++ std::complex. All functions supported are listed in sycl/doc/extensions/C-CXX-StandardLibrary/C-CXX-StandardLibrary.rst.

vladimirlaz
vladimirlaz previously approved these changes Dec 26, 2019
View reviewed changes
Copy link
Contributor

@vladimirlaz vladimirlaz left a comment

Choose a reason for hiding this comment

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

approve to trigger testing

@vladimirlaz vladimirlaz self-requested a review December 26, 2019 08:29
@vladimirlaz vladimirlaz dismissed their stale review December 26, 2019 08:30

approved to trigger testing

@vladimirlaz vladimirlaz removed their request for review December 26, 2019 08:30
@asavonic asavonic requested a review from romanovvlad December 26, 2019 09:47
asavonic
asavonic reviewed Dec 26, 2019
View reviewed changes
@jinge90 jinge90 force-pushed the devicelib_cmath_complex branch 2 times, most recently from 13796a6 to 462b76d Compare December 27, 2019 11:58
asavonic
asavonic suggested changes Dec 30, 2019
View reviewed changes
Copy link
Contributor

@asavonic asavonic left a comment

Choose a reason for hiding this comment

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

@jinge90, make sure this patch doesn't introduce any regressions.

@jinge90 jinge90 force-pushed the devicelib_cmath_complex branch 2 times, most recently from f09e341 to 0bdb7d3 Compare January 7, 2020 08:35
bader
bader reviewed Jan 9, 2020
View reviewed changes
@jinge90 jinge90 force-pushed the devicelib_cmath_complex branch from 0bdb7d3 to cba4ed7 Compare January 10, 2020 07:05
bader
bader reviewed Jan 10, 2020
View reviewed changes
@bader bader requested a review from asavonic January 10, 2020 09:02
@jinge90 jinge90 force-pushed the devicelib_cmath_complex branch from cba4ed7 to a91add6 Compare January 14, 2020 12:11
@jinge90
Copy link
Contributor Author

jinge90 commented Jan 14, 2020

Hi, @bader @asavonic , @andykaylor
I made a little investigation today and find it is not good to add any device library into clang driver's linking process as default. All device libraries(the wrapper libraries such as libsycl-glibc/cmath/complex) contain LLVM IR code for our wrapper functions and will be linked with users' device code by llvm-link before the llvm-spirv translation if we add them as default. And llvm-link will put everything into final LLVM bc file and then into final SPIR-V used for jit during runtime no matter whether those functions are used by users or not. Although those wrapper libraries don't contain as much code as fallback device library, they may still have big impact to some very small SYCL kernels because we have larger SPIR-V image and the jit overhead will increase. What's more important, I think not many users have strong requirement for those device libraries.
If someone really want to use any device libraries, they can manually link the library with their code.
But we need to document how to use the device library in details.
Thank you very much.

@jinge90 jinge90 force-pushed the devicelib_cmath_complex branch from a91add6 to c14973c Compare January 30, 2020 13:01
@jinge90
Copy link
Contributor Author

jinge90 commented Jan 30, 2020

@jinge90, make sure this patch doesn't introduce any regressions.
Hi, @asavonic
This regression is due to the overhead of sycl runtime loading fallback spirv and current device library infrastructure can't fix this as we will always load all fallback spv files no matter whether they are really needed or not.
I will try to parse user's device code to see if any dependency on device library exists or not and we can skip all device library if they are not needed by user's code.
As only one case on CPU platform are impacted, can we submit this patch first?

@jinge90 jinge90 force-pushed the devicelib_cmath_complex branch from c14973c to c4ec5ec Compare February 6, 2020 15:14
@jinge90
Copy link
Contributor Author

jinge90 commented Feb 7, 2020

Hi, all. I updated the patch and split device libraries, fallback spir-v libraries and devicelib extension into 2 parts for float and double support. sycl runtime will load fallback spir-v libraries for double support only if underlying device has "cl_khr_fp64" extension, this should fix the problem on some platform without fp64 support.
Hi, @asavonic , could you help me review whether the changes in program_manager.cpp are correct or not? Thank you very much.

asavonic
asavonic previously approved these changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@asavonic asavonic 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 Feb 10, 2020

@jinge90, could you resolve, merge conflict, please?

@jinge90 jinge90 force-pushed the devicelib_cmath_complex branch from c4ec5ec to 2ad7dfe Compare February 11, 2020 06:00
@jinge90
Copy link
Contributor Author

jinge90 commented Feb 11, 2020

@jinge90, could you resolve, merge conflict, please?

Hi, @bader
I have rebased and solved the merge conflicts.
Thank you very much.

@bader bader requested a review from asavonic February 11, 2020 08:49
@bader bader merged commit 7abd9d5 into intel:sycl Feb 11, 2020
@jinge90 jinge90 deleted the devicelib_cmath_complex branch February 12, 2020 02:57
s-kanaev
s-kanaev reviewed Feb 12, 2020
View reviewed changes
sycl/source/detail/devicelib/device_math.h
#ifndef __SYCL_CMATH_WRAPPER_H__
#define __SYCL_CMATH_WRAPPER_H__

double __spirv_ocl_log(double);
Copy link
Contributor

Choose a reason for hiding this comment

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

All these declarations should reside in spirv_ops.hpp, shouldn't they?

bader
bader reviewed Mar 30, 2020
View reviewed changes
sycl/source/detail/devicelib/CMakeLists.txt
add_custom_target(devicelib-obj DEPENDS ${devicelib-obj-file})
add_custom_target(devicelib-spv DEPENDS ${binary_dir}/libsycl-fallback-cassert.spv)
add_custom_command(OUTPUT ${binary_dir}/libsycl-fallback-complex.spv
COMMAND ${clang} -S -fsycl-device-only -fno-sycl-use-bitcode
Copy link
Contributor

@bader bader Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should remove -S here and from all custom commands below.
If I understand it correctly, today compiler doesn't support it, but it implies textual format of SPIR-V file whereas this command must produce binary format.

@jinge90 jinge90 mentioned this pull request Apr 30, 2021
Merged
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
@JackAKirk
[SYCL][CUDA] Explicitly adding extension headers in tests. (intel/llv…
d1a9f42 
…m-test-suite#1072)

Explicitly including the extension headers since tests are complaining about missing extension functions/classes in : intel#975 (comment).

Signed-off-by: JackAKirk [email protected]
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

@s-kanaev s-kanaev s-kanaev left review comments

@asavonic asavonic asavonic approved these changes

@vladimirlaz vladimirlaz vladimirlaz left review comments

@andykaylor andykaylor Awaiting requested review from andykaylor

@romanovvlad romanovvlad Awaiting requested review from romanovvlad

Assignees

@asavonic asavonic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jinge90 @bader @asavonic @vladimirlaz @andykaylor @s-kanaev