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

jinge90 · 2021-02-25T07:02:44Z

No description provided.

jinge90 · 2021-02-25T07:03:37Z

std::div depends on C function div from , this PR aims to support in SYCL device.

v-klochkov · 2021-02-25T16:42:24Z

Would you add a LIT test please

vzakhari · 2021-02-25T18:26:28Z

libdevice/crt_wrapper.cpp

@@ -9,6 +9,14 @@
 #include "wrapper.h"

 #ifdef __SPIR__
+DEVICE_EXTERN_C


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.

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.

vzakhari · 2021-02-25T18:27:56Z

libdevice/crt_wrapper.cpp

@@ -9,6 +9,14 @@
 #include "wrapper.h"

 #ifdef __SPIR__
+DEVICE_EXTERN_C
+div_t div(int x, int y) { return {x / y, x % y}; }


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.

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.

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.

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.

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

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

jinge90 · 2021-02-26T05:34:48Z

Would you add a LIT test please
Hi, @v-klochkov
All SYCL DeviceLib lit tests have been moved to https://github.com/intel/llvm-test-suite , so should I commit the lit test to llvm-test-suite repo directly or do we have some process to add such lit test?
Thanks.

v-klochkov · 2021-02-26T05:39:40Z

Would you add a LIT test please
Hi, @v-klochkov
All SYCL DeviceLib lit tests have been moved to https://github.com/intel/llvm-test-suite , so should I commit the lit test to llvm-test-suite repo directly or do we have some process to add such lit test?
Thanks.

Please create a PR with a tests for DEVICE at llvm-test-suite and add a link to that PR here. And vice-versa, add a link to this PR in the LIT-test-PR.

jinge90 · 2021-02-26T06:56:18Z

Would you add a LIT test please
Hi, @v-klochkov
All SYCL DeviceLib lit tests have been moved to https://github.com/intel/llvm-test-suite , so should I commit the lit test to llvm-test-suite repo directly or do we have some process to add such lit test?
Thanks.

Please create a PR with a tests for DEVICE at llvm-test-suite and add a link to that PR here. And vice-versa, add a link to this PR in the LIT-test-PR.

The lit test PR is: intel/llvm-test-suite#159

AlexeySachkov

sycl-post-link changes LGTM

libdevice/fallback-cmath.cpp

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

vzakhari · 2021-03-03T16:06:56Z

libdevice/device_math.h

@@ -10,9 +10,18 @@
 #define __LIBDEVICE_DEVICE_MATH_H__

 #include "device.h"
-
+#include <cstdlib>


Please move this under __SPIR__ below.

kbobrovs

LGTM. I assume sycl-post-link changes will eventually go away together with the rest of DeviceLibFuncMap, as a part of #3291

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

jinge90 · 2021-03-04T14:59:24Z

This patch need rebase and resolve some conflicts with latest sycl-post-link.cpp.

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

jinge90 · 2021-03-05T02:05:29Z

Hi, @vzakhari and @kbobrovs
I rebased the patch resolving conflicts with latest sycl-post-link.cpp and may need your approval again.
Thanks very much.

jinge90 · 2021-03-09T05:35:41Z

Hi, @AlexeySachkov and @intel/llvm-reviewers-runtime
Could you help review this PR?
Thanks very much.

jinge90 · 2021-03-09T05:35:57Z

/summary:run

v-klochkov

Looks good to me.

I suppose there are no rules requiring uplifting MINOR version of the library for libdevice when to ABI non-breaking changes (add new external symbols) as it is required for SYCL library, right?

romanovvlad · 2021-03-09T10:18:43Z

Looks good to me.

I suppose there are no rules requiring uplifting MINOR version of the library for libdevice when to ABI non-breaking changes (add new external symbols) as it is required for SYCL library, right?

Correct.

support div/ldiv/lldiv from <stdlib.h> in libdevice

784895a

jinge90 requested review from vzakhari and a team as code owners February 25, 2021 07:02

jinge90 requested a review from v-klochkov February 25, 2021 07:02

vzakhari reviewed Feb 25, 2021

View reviewed changes

jinge90 requested review from AlexeySachkov and kbobrovs as code owners February 26, 2021 05:26

Move actual implementation of div into fallback lib

6f13dd5

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

jinge90 mentioned this pull request Feb 26, 2021

[SYCL] Add lit test for integer math function div, ldiv, lldiv intel/llvm-test-suite#159

Merged

AlexeySachkov previously approved these changes Feb 26, 2021

View reviewed changes

vzakhari reviewed Feb 26, 2021

View reviewed changes

libdevice/fallback-cmath.cpp Show resolved Hide resolved

jinge90 dismissed AlexeySachkov’s stale review via 5967d33 March 1, 2021 01:25

jinge90 added 3 commits March 1, 2021 09:32

add wrapper obj impl

5967d33

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

clang format the patch

e035475

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

clang-format patch

54717e3

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

vzakhari reviewed Mar 3, 2021

View reviewed changes

kbobrovs previously approved these changes Mar 4, 2021

View reviewed changes

jinge90 dismissed kbobrovs’s stale review via 375416d March 4, 2021 03:32

move cstdlib inclusion under __SPIR__

375416d

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

vzakhari previously approved these changes Mar 4, 2021

View reviewed changes

jinge90 dismissed vzakhari’s stale review via 77b7cf2 March 5, 2021 01:51

rebase to latest sycl-post-link

77b7cf2

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

jinge90 requested a review from kbobrovs March 5, 2021 02:05

jinge90 requested review from vzakhari and AlexeySachkov March 5, 2021 02:05

vzakhari approved these changes Mar 5, 2021

View reviewed changes

kbobrovs approved these changes Mar 5, 2021

View reviewed changes

v-klochkov approved these changes Mar 9, 2021

View reviewed changes

romanovvlad merged commit 2e9d33c into intel:sycl Mar 9, 2021

jinge90 mentioned this pull request Apr 30, 2021

[SYCL][DOC] DeviceLib doc update to add new supported items #3623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jinge90 commented Feb 25, 2021

jinge90 commented Feb 25, 2021

v-klochkov commented Feb 25, 2021

vzakhari Feb 25, 2021

jinge90 Feb 26, 2021

vzakhari Feb 25, 2021

jinge90 Feb 26, 2021

vzakhari Feb 26, 2021

jinge90 Mar 1, 2021

vzakhari Mar 3, 2021

jinge90 commented Feb 26, 2021

v-klochkov commented Feb 26, 2021

jinge90 commented Feb 26, 2021

AlexeySachkov left a comment

vzakhari Mar 3, 2021

kbobrovs left a comment

jinge90 commented Mar 4, 2021

jinge90 commented Mar 5, 2021

jinge90 commented Mar 9, 2021

jinge90 commented Mar 9, 2021

v-klochkov left a comment

romanovvlad commented Mar 9, 2021

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

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

Conversation

jinge90 commented Feb 25, 2021

jinge90 commented Feb 25, 2021

v-klochkov commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinge90 commented Feb 26, 2021

v-klochkov commented Feb 26, 2021

jinge90 commented Feb 26, 2021

AlexeySachkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbobrovs left a comment

Choose a reason for hiding this comment

jinge90 commented Mar 4, 2021

jinge90 commented Mar 5, 2021

jinge90 commented Mar 9, 2021

jinge90 commented Mar 9, 2021

v-klochkov left a comment

Choose a reason for hiding this comment

romanovvlad commented Mar 9, 2021