Skip to content

[SYCL] Support stdlib function div, ldiv, lldiv in device library #3262

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

Merged
romanovvlad merged 7 commits into from
Mar 9, 2021
Merged

[SYCL] Support stdlib function div, ldiv, lldiv in device library #3262

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
784895a
support div/ldiv/lldiv from <stdlib.h> in libdevice
jinge90 Feb 25, 2021
6f13dd5
Move actual implementation of div into fallback lib
jinge90 Feb 26, 2021
5967d33
add wrapper obj impl
jinge90 Mar 1, 2021
e035475
clang format the patch
jinge90 Mar 1, 2021
54717e3
clang-format patch
jinge90 Mar 1, 2021
375416d
move cstdlib inclusion under __SPIR__
jinge90 Mar 4, 2021
77b7cf2
rebase to latest sycl-post-link
jinge90 Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions libdevice/crt_wrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@
#include "wrapper.h"

#ifdef __SPIR__
DEVICE_EXTERN_C
Copy link
Contributor

@vzakhari vzakhari Feb 25, 2021

Choose a reason for hiding this comment

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

I guess we'd better have a dedicated integer math file instead of placing this in the crt portion, but we can do the clean-up later.

Copy link
Contributor Author

@jinge90 jinge90 Feb 26, 2021

Choose a reason for hiding this comment

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

I guess we'd better have a dedicated integer math file instead of placing this in the crt portion, but we can do the clean-up later.

It seems that "div" is declared in instead of but it makes sense to group all "math related" items into one math file. I will update it later.
Thanks.

div_t div(int x, int y) { return {x / y, x % y}; }
Copy link
Contributor

@vzakhari vzakhari Feb 25, 2021

Choose a reason for hiding this comment

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

We have to allow device runtimes to provide their optimized versions of these functions, so we have to call __devicelib_ variants here, and put the actual implementation into the fallback part of the library.

Copy link
Contributor Author

@jinge90 jinge90 Feb 26, 2021

Choose a reason for hiding this comment

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

We have to allow device runtimes to provide their optimized versions of these functions, so we have to call __devicelib_ variants here, and put the actual implementation into the fallback part of the library
To some complicated functions such as __divsc3/__mulsc3, putting the actual implementation into fallback spv is valuable since device runtime may have interest providing a more optimized version. To div, it looks like the implementation is too simple, I doubt whether device runtime would like to provide a more optimized version. If device runtime wouldn't like or there is not much value to optimize such "simple" functions, just providing the actual implementation in wrapper obj may be conciser.
Thanks very much.

Copy link
Contributor

@vzakhari vzakhari Feb 26, 2021

Choose a reason for hiding this comment

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

We have to allow device runtimes to provide their optimized versions of these functions, so we have to call __devicelib_ variants here, and put the actual implementation into the fallback part of the library
To some complicated functions such as __divsc3/__mulsc3, putting the actual implementation into fallback spv is valuable since device runtime may have interest providing a more optimized version. To div, it looks like the implementation is too simple, I doubt whether device runtime would like to provide a more optimized version. If device runtime wouldn't like or there is not much value to optimize such "simple" functions, just providing the actual implementation in wrapper obj may be conciser.
Thanks very much.

I think there may be a hardware instruction that computes both values on some devices, so it does make sense to have devicelib api for these operations.

Copy link
Contributor Author

@jinge90 jinge90 Mar 1, 2021

Choose a reason for hiding this comment

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

We have to allow device runtimes to provide their optimized versions of these functions, so we have to call __devicelib_ variants here, and put the actual implementation into the fallback part of the library
To some complicated functions such as __divsc3/__mulsc3, putting the actual implementation into fallback spv is valuable since device runtime may have interest providing a more optimized version. To div, it looks like the implementation is too simple, I doubt whether device runtime would like to provide a more optimized version. If device runtime wouldn't like or there is not much value to optimize such "simple" functions, just providing the actual implementation in wrapper obj may be conciser.
Thanks very much.

I think there may be a hardware instruction that computes both values on some devices, so it does make sense to have devicelib api for these operations.

Hi, @vzakhari
If so, moving div into fallback spv makes sense. We have another libc item to add into devicelib which is also very simple:
int abs(int x);
Do you think we need to move its actual implementation into fallback lib or just provide the actual impl in wrapper obj?
I think the impl is very simple:
int abs(int x) { return i < 0 ? -i : i; }

Thanks very much.

Copy link
Contributor

@vzakhari vzakhari Mar 3, 2021

Choose a reason for hiding this comment

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

I think it is always easier to follow the common pattern/design, than to introduce exceptions :)


DEVICE_EXTERN_C
ldiv_t ldiv(long int x, long int y) { return {x / y, x % y}; }

DEVICE_EXTERN_C
lldiv_t lldiv(long long int x, long long int y) { return {x / y, x % y}; }
#if defined(_WIN32)
// Truncates a wide (16 or 32 bit) string (wstr) into an ASCII string (str).
// Any non-ASCII characters are replaced by question mark '?'.
Expand Down
1 change: 1 addition & 0 deletions libdevice/wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#include <cstddef>
#include <cstdint>
#include <cstdlib>
// We need the following header to ensure the definition of all spirv variables
// required by the wrapper libraries.
#include "spirv_vars.h"
Expand Down
5 changes: 5 additions & 0 deletions sycl/include/CL/sycl/builtins.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,6 +1544,11 @@ detail::enable_if_t<detail::is_genfloatf<T>::value, T> tan(T x) __NOEXC {
} // __SYCL_INLINE_NAMESPACE(cl)

#ifdef __SYCL_DEVICE_ONLY__
extern "C" {
extern SYCL_EXTERNAL div_t div(int x, int y);
extern SYCL_EXTERNAL ldiv_t ldiv(long int x, long int y);
extern SYCL_EXTERNAL lldiv_t lldiv(long long int x, long long int y);
}
#ifdef __GLIBC__
extern "C" {
extern SYCL_EXTERNAL void __assert_fail(const char *expr, const char *file,
Expand Down