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.

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

[SYCL] Add support for MSVC internal math functions in device library #1441

Merged
merged 6 commits into from
Apr 30, 2020

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Apr 1, 2020

Signed-off-by: gejin [email protected]
This patch is to add support for several MSVC internal functions in device libraries and the implementation of these MSVC internal functions are based on Microsoft STL: (https://github.com/microsoft/STL).

@jinge90
Copy link
Contributor Author

jinge90 commented Apr 1, 2020

Hi, @asavonic and @bader
This patch includes some MSVC internal math functions which are used in MSVC std::complex implementation. Could you help me review?
Thank you very much.

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

You must add some tests for the new code.

libdevice/cmath_wrapper.cpp Show resolved Hide resolved
libdevice/cmath_wrapper.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper_fp64.cpp Show resolved Hide resolved
@jinge90
Copy link
Contributor Author

jinge90 commented Apr 7, 2020

You must add some tests for the new code.

Hi, Alexey.
We have another PR: #1256 which includes cmath function test. Could we merge that patch first and we can add msvc math functions test in current cmath test?
Thank you very much.

libdevice/cmath_wrapper.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper_fp64.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper_fp64.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper_fp64.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper_fp64.cpp Outdated Show resolved Hide resolved
libdevice/cmath_wrapper_fp64.cpp Outdated Show resolved Hide resolved
@jinge90
Copy link
Contributor Author

jinge90 commented Apr 16, 2020

You must add some tests for the new code.

Add cases in math_windows_test and math_fp64_windows_test for those MSVC internal functions.

@romanovvlad
Copy link
Contributor

@bader Could you please answer?

@bader
Copy link
Contributor

bader commented Apr 21, 2020

@bader Could you please answer?

What is the question?

@romanovvlad
Copy link
Contributor

@bader Could you please answer?

What is the question?

Sorry, was looking at the old version of the page.

sycl/include/CL/sycl/builtins.hpp Show resolved Hide resolved
double a = NAN;
res_access[i++] = tgamma(a);
res_access[i++] = lgamma(a);
enm_access[0] = _Dtest(&a);
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not look right that we reference an implementation-defined name in a test. We should instead call some standard math function that uses _Dtest (and others).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @vzakhari
In fact, those functions are MSVC internal math functions and I think MSVC standard math functions hasn't used those functions as we only found those functions are required in MSVC std::complex implementation. In fact, if we build and run complex tests on Windows, those internal functions will be covered.
I prefer to testing all functions added to device library. Do you have any comments? @bader

@jinge90 jinge90 requested a review from a team as a code owner April 28, 2020 10:11
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

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

I'm OK with changes in sycl/include/CL/sycl/builtins.hpp

@bader bader merged commit fd14167 into intel:sycl Apr 30, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request May 6, 2020
…_docs

* origin/sycl: (6482 commits)
  [SYCL][NFC] Clean formatting in Markdown documents (intel#1635)
  [SYCL][Doc] Remove obsolete parens from README (intel#1637)
  [SYCL] Fix failing ABI tests when LLVM_LIBDIR_SUFFIX is set (intel#1605)
  [SYCL] Fix warnings in libdevice (intel#1630)
  [SYCL][CUDA] Triage and clean LIT (intel#1620)
  [SYCL][NFC] Fix GCC 8 compilation warnings (intel#1631)
  [SYCL] Minor fixes in LowerWGScope
  [SYCL] PI: correct default interoperability plugin selection
  [SYCL] Add faster reduction implementations using atomic or/and intel::reduce() (intel#1615)
  [SYCL] Add sycl-ls utility for listing devices discovered/selected by SYCL RT (intel#1575)
  [SYCL] Fix getDeviceFromHandler declarations (intel#1626)
  [SPIR-V] Correct/improve declaration of SPIR-V builtins (intel#1519)
  [SYCL][USM] Improve USM allocator test and fix improper behavior. (intel#1538)
  [SYCL] Fix failing ABI LITs (intel#1622)
  [SYCL] Add support for MSVC internal math functions in device library (intel#1441)
  [SYCL] Add runtime library versioning (intel#1604)
  [SYCL] Check weak symbols in ABI dumps (intel#1609)
  [NFC][SYCL] Improve kernel metadata test (intel#1610)
  Revert "[SYCL] XFAIL LIT test due to duplicate diagnostic" (intel#1460)
  [SYCL] Move the reduction command group funcs out of handler.hpp (intel#1602)
  ...
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.

4 participants