[SYCL][Test] Devicelib test #1256

haonanya · 2020-03-06T03:37:42Z

add test for all math function.
In llvm/sycl/source/detail/devicelib/CMakeLists.txt, libsycl-cmath.o depends on device_math.h,
modify here.

Signed-off-by: haonanya <[email protected]>

…math.o depends on device_math.h, modify here Signed-off-by: haonanya <[email protected]>

jinge90 · 2020-03-06T09:30:45Z

Hi, @bader , @asavonic , @andykaylor
We plan to upload more tests for cmath device library, could you help review?
Thank you very much.

sycl/test/devicelib/math_test.cpp

Signed-off-by: haonanya <[email protected]>

…t.cpp, as math_utils.hpp changes Signed-off-by: haonanya <[email protected]>

Signed-off-by: haonanya <[email protected]>

…math_utils.hpp Signed-off-by: haonanya <[email protected]>

Signed-off-by: haonanya <[email protected]>

sycl/test/devicelib/math_utils.hpp

bader · 2020-03-12T14:55:54Z

sycl/test/devicelib/math_utils.hpp

-    else {
+      T max_v = std::fmax(std::abs(x), std::abs(y));
+      return (std::abs(x - y) / max_v) <
+             std::numeric_limits<T>::epsilon() * 100;


Suggested change

std::numeric_limits<T>::epsilon() * 100;

std::numeric_limits<T>::epsilon() * 100;

Why 100?
Different math functions have different accuracy requirements and this seems to be too relax.

The bigger problem is that single threshold is applied to all functions. I think it should the threshold should be function dependent. Not sure if we want to checks the requirements set by the specification, but current value seems to be too relaxed.

Hi, Alexey.
In fact, we added those cases only to test basic functionality of math functions provided by device library. The accuracy is not meant to be covered by these tests, all those input values of cmath functions are very simple ones. In fact, we are doing some accuracy tests on device library's and , all input will be much more complicated and different threshold are used and compared.
Thank you very much.

sycl/test/devicelib/math_test.cpp

Signed-off-by: haonanya <[email protected]>

bader · 2020-03-16T08:50:05Z

sycl/test/devicelib/math_utils.hpp

+  // At least one input is nan
+  if (std::isnan(x) || std::isnan(y))
+    ret = std::isnan(x) && std::isnan(y);
+
+  // At least one input is inf
+  else if (std::isinf(x) || std::isinf(y))


I find the code with early exit more readable than this version.

Suggested change

// At least one input is nan

if (std::isnan(x) || std::isnan(y))

ret = std::isnan(x) && std::isnan(y);

// At least one input is inf

else if (std::isinf(x) || std::isinf(y))

// At least one input is nan

if (std::isnan(x) || std::isnan(y))

return std::isnan(x) && std::isnan(y);

// At least one input is inf

if (std::isinf(x) || std::isinf(y))

Fix for fixing this, but could you apply this comment to the file, please?

Hi, Alexey.
I am sorry to make things complicated. I am new user.
Thanks for your patience.

bader · 2020-03-16T08:52:18Z

Please, fix code formatting issues.

Signed-off-by: haonanya <[email protected]>

keryell · 2020-03-17T21:54:42Z

sycl/test/devicelib/math_utils.hpp

@@ -1,25 +1,33 @@
 #ifndef MATH_UTILS
 #include <complex>
 #include <limits>
-using namespace std;
 // T must be float-point type
 template <typename T>
 bool is_about_FP(T x, T y) {


While you are here, could you add a comment explaining what this function... is_about? :-)

Hi, Ronan.
Since it is not proper to compare float point using operator ==, this function measure whether the result of cmath function from kernel is close to the reference and machine epsilon is used as threshold in this function.
Thanks very much.

Ah quite clearer with a comment! :-)

Then perhaps a clearer name for the function would help...

fuzzy_equal_fp ? approx_equal_fp ? I am not an native English speaker so I am not very helpful here...

sycl/test/devicelib/math_utils.hpp

Modify code to follow coding standards Co-Authored-By: Ronan Keryell <[email protected]> Signed-off-by: haonanya <[email protected]>

Signed-off-by: haonanya <[email protected]>

keryell · 2020-03-18T22:31:39Z

sycl/test/devicelib/math_utils.hpp

@@ -1,25 +1,33 @@
 #ifndef MATH_UTILS
 #include <complex>
 #include <limits>
-using namespace std;
 // T must be float-point type
 template <typename T>
 bool is_about_FP(T x, T y) {


Ah quite clearer with a comment! :-)

Then perhaps a clearer name for the function would help...

fuzzy_equal_fp ? approx_equal_fp ? I am not an native English speaker so I am not very helpful here...

keryell · 2020-03-18T22:32:59Z

sycl/test/devicelib/math_utils.hpp

    }
  }
+  return ret;


In this kind of code it is probably clearer to remove ret and just have as many early return as required.

Thank for your comment, early return is applied to new commit and approx_equal_fp is used as the name of float point comparison.

… return. Signed-off-by: haonanya <[email protected]>

Signed-off-by: haonanya <[email protected]>

….hpp, Modify corresponding call in these files. Signed-off-by: haonanya <[email protected]>

Signed-off-by: haonanya <[email protected]>

bader · 2020-03-22T11:09:13Z

@haonanya, please, resolve conflicts.

Signed-off-by: haonanya <[email protected]>

bader

One minor comment.
Other than that - looks good to me.

bader · 2020-04-07T10:41:50Z

sycl/test/devicelib/math_utils.hpp

+  if (std::isinf(x) || std::isinf(y))
+    return (x == y);
+
+  // two finite
  else {


Please, don't use else after return.
Apply here and all the cases below.

Signed-off-by: haonanya <[email protected]>

bader · 2020-04-07T14:38:05Z

LGTM, but I'd like to have reviews from @asavonic and @romanovvlad.
Thanks!

asavonic · 2020-04-08T16:35:56Z

sycl/test/devicelib/cmath_fp64_test.cpp

@@ -0,0 +1,113 @@
+// UNSUPPORTED: windows
+// RUN: %clangxx -fsycl -c %s -o %t.o
+// RUN: %clangxx -fsycl %t.o %sycl_libs_dir/libsycl-cmath-fp64.o -o %t.out


Please add a RUN line for execution here and in all other tests.
Otherwise this just tests for compilation and no runtime validation is performed.

If these are going to run the compiled program, they should be moved to end-to-end testing.

Hi, @andykaylor and @asavonic
Could we enable the run command for cmath tests on CPU device currently?
For GPU, only OCL RT can work. For CPU, all cmath and complex tests should work in theory but OCL CPU RT has an issue which will lead to crash on some platforms, the fix has not been merged into latest OCL CPU RT, so we may encounter some unexpected crash if enable complex tests.
Thank you very much.

@andykaylor

If these are going to run the compiled program, they should be moved to end-to-end testing.

Most of the tests in sycl/test directory have RUN lines to test on available devices.
This test in particular has a lot of runtime asserts.

Could we enable the run command for cmath tests on CPU device currently?

I think they should be enabled for all devices you expect this code to work. I assume it should work for CPU and GPU, right? FPGA is probably not going work, because AOT compilation is not support for device libraries yet.

If a test doesn't work on some platform due to a bug, it should be disabled temporarily.

Signed-off-by: haonanya <[email protected]>

bader · 2020-04-10T15:23:32Z

@asavonic, ping.

…c_abi_checks * origin/sycl: [SYCL][Driver] Enforce unique filenames when -save-temps is used (intel#1545) [SYCL] [xmethods] Allow replacing xmethod script (intel#1532) [SYCL] Add tests for inline asm feature (intel#1444) [SYCL][Doc] Add device_specific_kernel_queries extension. (intel#1540) [SYCL][USM] Remove unused header and unnecessary includes (intel#1537) Fix check-llvm dependencies (intel#1547) [SYCL] Add __SYCL_EXPORT to declaration of contextSetExtendedDeleter (intel#1531) [SYCL][Doc] Add static local memory query extension. (intel#1539) [SYCL][Doc] Add sycl_bitcast extension (intel#1541) [SYCL][NFC] Temporarily disable sporadically failing test (intel#1533) [SYCL][NFC] Adjust codeowners for sycl directory (intel#1529) [SYCL] Fix processing of spec consts referenced twice (intel#1524) [SYCL] Use correct macro name in export.hpp (intel#1527) [Driver][NFC] Fix -help information for -Xs options (intel#1530) [SYCL][Doc] Add Graph Scheduler design documentation (intel#1457) [SYCL] Add diagnostics for long double in device code (intel#1512) [SYCL] Add a mutex to state-modifying program functions (intel#1204) [SYCL][Test] Add Devicelib tests (intel#1256) [SYCL] Refactor semantic checks for variable types (intel#1513)

The types of lower bound, upper bound, and step are converted into the type of the loop variable if necessary. OpenMP runtime requires 32-bit or 64-bit loop variables. OpenMP loop iteration variable cannot have more than 64 bits size and will be narrowed. This patch is part of upstreaming code from the fir-dev branch of https://github.com/flang-compiler/f18-llvm-project. (#1256) Co-authored-by: kiranchandramohan <[email protected]> Reviewed By: kiranchandramohan, shraiysh Differential Revision: https://reviews.llvm.org/D125740

haonanya1 added 2 commits March 6, 2020 11:13

[SYCL] add test for all math function.

c3a5cd5

Signed-off-by: haonanya <[email protected]>

[SYCL] in llvm/sycl/source/detail/devicelib/CMakeLists.txt, libsycl-c…

1816430

…math.o depends on device_math.h, modify here Signed-off-by: haonanya <[email protected]>

haonanya changed the title ~~[SYCL] Devicelib test~~ [SYCL][Test] Devicelib test Mar 6, 2020

jinge90 requested review from bader, jinge90, andykaylor and asavonic March 6, 2020 09:27

bader requested changes Mar 7, 2020

View reviewed changes

sycl/test/devicelib/math_test.cpp Outdated Show resolved Hide resolved

sycl/test/devicelib/math_test.cpp Outdated Show resolved Hide resolved

haonanya1 added 6 commits March 9, 2020 20:33

[SYCL] Put all math function calls into a single kernel.

5540691

Signed-off-by: haonanya <[email protected]>

[SYCL] Remove using namespace std in math_utils.hpp

79dce3e

Signed-off-by: haonanya <[email protected]>

[SYCL] modify std_complex_math_fp64_test.cpp and std_complex_math_tes…

3836b41

…t.cpp, as math_utils.hpp changes Signed-off-by: haonanya <[email protected]>

[SYCL] [Test] Put all math function calls into a single kernel

e28efb4

Signed-off-by: haonanya <[email protected]>

[SYCL] [Test] Deal with nan and inf input in is_about_FP function in …

1251e56

…math_utils.hpp Signed-off-by: haonanya <[email protected]>

[SYCL] Format code

9d62247

Signed-off-by: haonanya <[email protected]>

bader reviewed Mar 12, 2020

View reviewed changes

[SYCL] Code clean

b670056

Signed-off-by: haonanya <[email protected]>

bader reviewed Mar 16, 2020

View reviewed changes

bader previously approved these changes Mar 16, 2020

View reviewed changes

haonanya dismissed bader’s stale review via fd16ef2 March 16, 2020 09:37

[SYCL] [Test] Improve readability for math_utils.hpp

fd16ef2

Signed-off-by: haonanya <[email protected]>

keryell reviewed Mar 17, 2020

View reviewed changes

haonanya and others added 2 commits March 18, 2020 11:38

Update sycl/test/devicelib/math_utils.hpp

764518e

Modify code to follow coding standards Co-Authored-By: Ronan Keryell <[email protected]> Signed-off-by: haonanya <[email protected]>

[SYCL] Add comment for is_about_FP function in math_utils.hpp

10c938b

Signed-off-by: haonanya <[email protected]>

romanovvlad assigned asavonic Mar 18, 2020

keryell reviewed Mar 18, 2020

View reviewed changes

haonanya1 added 3 commits March 19, 2020 11:25

[SYCL] [Test] Rename is_about_FP function and remove ret to use early…

e8bfe40

… return. Signed-off-by: haonanya <[email protected]>

[SYCL] [Test] Rename complex compare function.

014d3b2

Signed-off-by: haonanya <[email protected]>

[SYCL] [Test] There is a rename for float point compare in math_utils…

6ef6fac

….hpp, Modify corresponding call in these files. Signed-off-by: haonanya <[email protected]>

Merge branch 'sycl' into devicelibTest

0e84acf

Signed-off-by: haonanya <[email protected]>

Merge branch 'sycl' into devicelibTest

d6cd3fc

Signed-off-by: haonanya <[email protected]>

jinge90 mentioned this pull request Apr 7, 2020

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

Merged

bader reviewed Apr 7, 2020

View reviewed changes

Fix use else after return issue

88fc254

Signed-off-by: haonanya <[email protected]>

haonanya requested a review from romanovvlad as a code owner April 7, 2020 13:19

romanovvlad previously approved these changes Apr 8, 2020

View reviewed changes

asavonic suggested changes Apr 8, 2020

View reviewed changes

haonanya dismissed romanovvlad’s stale review via 048e5d0 April 9, 2020 06:28

haonanya1 added 3 commits April 9, 2020 14:30

[SYCL][Test] Enable cmath lit test.

048e5d0

Signed-off-by: haonanya <[email protected]>

[SYCL][Test] Enable Host device for math_override.cpp.

e8ac45e

Signed-off-by: haonanya <[email protected]>

[SYCL] [Test] Fix format issue.

a54c5f1

Signed-off-by: haonanya <[email protected]>

bader requested a review from asavonic April 9, 2020 12:22

asavonic approved these changes Apr 15, 2020

View reviewed changes

bader approved these changes Apr 15, 2020

View reviewed changes

bader merged commit ec80987 into intel:sycl Apr 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][Test] Devicelib test #1256

[SYCL][Test] Devicelib test #1256

haonanya commented Mar 6, 2020

jinge90 commented Mar 6, 2020

bader Mar 12, 2020

jinge90 Mar 16, 2020

bader Mar 16, 2020 •

edited

Loading

bader Mar 16, 2020

haonanya Mar 16, 2020

bader commented Mar 16, 2020

keryell Mar 17, 2020

haonanya Mar 18, 2020

keryell Mar 18, 2020

keryell Mar 18, 2020

keryell Mar 18, 2020

haonanya Mar 19, 2020

bader commented Mar 22, 2020

bader left a comment

bader Apr 7, 2020

bader commented Apr 7, 2020

asavonic Apr 8, 2020

andykaylor Apr 9, 2020

jinge90 Apr 9, 2020 •

edited

Loading

asavonic Apr 9, 2020

asavonic Apr 9, 2020

bader commented Apr 10, 2020

	std::numeric_limits<T>::epsilon() * 100;
	std::numeric_limits<T>::epsilon() * 100;

[SYCL][Test] Devicelib test #1256

[SYCL][Test] Devicelib test #1256

Conversation

haonanya commented Mar 6, 2020

jinge90 commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader commented Mar 16, 2020

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

bader commented Mar 22, 2020

bader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader commented Apr 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinge90 Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader commented Apr 10, 2020

bader Mar 16, 2020 •

edited

Loading

jinge90 Apr 9, 2020 •

edited

Loading