Skip to content

Added missing math APIs for devicelib. #2558

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 7 commits into from
Oct 5, 2020
Merged

Added missing math APIs for devicelib. #2558

merged 7 commits into from
Oct 5, 2020

Conversation

vzakhari
Copy link
Contributor

The new APIs are mostly for Windows, but I added double-precision scalbn for both Linux and Windows.

The Microsoft APIs are defined here: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/floating-point-primitives?view=vs-2019

There are still some Microsoft APIs missing.

Signed-off-by: Vyacheslav Zakharin [email protected]

@vzakhari vzakhari requested a review from jinge90 September 28, 2020 22:33
@vzakhari vzakhari requested review from asavonic, bader and a team as code owners September 28, 2020 22:33
Vyacheslav Zakharin added 2 commits September 28, 2020 15:57
@vzakhari vzakhari requested a review from erichkeane September 30, 2020 00:37
Vyacheslav Zakharin added 4 commits September 30, 2020 12:37
Avoid 'readnone' for _[f]dtest builtins.
7761d25 
Signed-off-by: Vyacheslav Zakharin <[email protected]>
Declare MSFT APIs in builtins.hpp.
5f6e2e3 
Signed-off-by: Vyacheslav Zakharin <[email protected]>
Avoid potential undefined behavior in _[f]dtest.
0e8fd04 
Signed-off-by: Vyacheslav Zakharin <[email protected]>
clang-format
94a833f 
Signed-off-by: Vyacheslav Zakharin <[email protected]>
@vzakhari
Copy link
Contributor Author

@bader, the changes here do not add any value to llvm-project, so they will probably not be accepted. Can FE people speak up on this? I am also not sure how to test this change, since the produced IR is not affected by the recognition of these builtins.

At the same time, I believe adding the builtins here is the right solution. Since SYCL FE uses getBuiltinID to allow builtins inside kernels, we need to expand this list instead of expanding the list in builtins.hpp.

I figured out that FE sets nounwind attribute for these builtins, so I should probably be able to create a clang test for them. This also shows some value in recognizing them as builtins, so I will try to push this into llvm-project. For the time being I will use builtins.hpp as a workaround.

bader
bader approved these changes Oct 1, 2020
View reviewed changes
sycl/test/devicelib/cmath_fp64_test.cpp
@@ -47,6 +47,12 @@ void device_cmath_test(s::queue &deviceQueue) {
auto quo_access = buffer4.template get_access<sycl_write>(cgh);
cgh.single_task<class DeviceMathTest>([=]() {
int i = 0;
T nan = NAN;
T minus_nan = -NAN;
T infinity = INFINITY;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that this test uses C macro instead of std::numeric_limits<T> methods intentionally to check the compatibility with "C"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. The C macro is just shorter to write.

@vzakhari
Copy link
Contributor Author

vzakhari commented Oct 2, 2020

Friendly ping...

@pvchupin pvchupin requested a review from againull October 5, 2020 18:40
againull
againull reviewed Oct 5, 2020
View reviewed changes
libdevice/fallback-cmath-fp64.cpp

DEVICE_EXTERN_C
double __devicelib_scalbn(double x, int exp) {
return __spirv_ocl_ldexp(x, exp);
Copy link
Contributor

@againull againull Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Am I correct that we assume here that FLT_RADIX is always == 2?
Should we make a comment about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we assume it is 2. Do you suggest mentioning this in the documentation for devicelib?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if it worth mentioning and how obvious is this that it is 100% equal to 2. If you think that it is obvious I am ok to leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to GCC documentation, it is 2 on the majority of the machines, so I would say it is quite expected to be equal to 2. At the same time, I was not able to find any documentation regarding floating point representation for SPIR-V devices.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thx.

@againull againull self-requested a review October 5, 2020 21:40
@pvchupin pvchupin merged commit e438bc8 into intel:sycl Oct 5, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Oct 7, 2020
Merge branch 'sycl' into global_constructors_wrapper
a0eeeff 
* sycl:
  [SYCL] Fix test failure due to size discrepancy in 32 and 64 bit environment (intel#2594)
  [BuildBot] Linux GPU driver uplift to 20.39.17972 (intel#2600)
  [SYCL][NFC] Remove cyclic dependency in headers (intel#2601)
  [SYCL][Doc] Fix Sphinx docs index (intel#2599)
  [SYCL][PI][L0] Add an assert to piEnqueueKernelLaunch() when the GlobalWorkOffset!=0 (intel#2593)
  [SYCL][Driver] Turn on -spirv-allow-extra-diexpressions option (intel#2578)
  [SYCL] Serialize queue related PI API in the Level-Zero plugin (intel#2588)
  Added missing math APIs for devicelib. (intel#2558)
  [SYCL][DOC] Fix path to FPGA device selector (intel#2563)
  [SYCL][CUDA] Add basic sub-group functionality (intel#2587)
  [SYCL] Add specialization constant feature design doc. (intel#2572)
  [SYCL] Expand release lit test to CPU and ACC (intel#2589)
  [SYCL] Add clang support for FPGA kernel attribute scheduler_target_fmax_mhz (intel#2511)
  [SYCL][ESIMD] Fixed compiler crash in LowerESIMDVecArg pass (intel#2556)
@jinge90 jinge90 mentioned this pull request Apr 30, 2021
Merged
kbenzie pushed a commit to kbenzie/intel-llvm that referenced this pull request Feb 17, 2025
@pbalcer
Merge pull request intel#2558 from EuphoricThinking/benchmark_patch_p…
02c82d4 
…arameter_for_tests

Add a benchmark scripts parameter for faster testing
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
@pbalcer
Merge pull request #2558 from EuphoricThinking/benchmark_patch_parame…
d430858 
…ter_for_tests

Add a benchmark scripts parameter for faster testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader approved these changes

@againull againull againull approved these changes

@jinge90 jinge90 Awaiting requested review from jinge90

@asavonic asavonic Awaiting requested review from asavonic

@sergey-semenov sergey-semenov Awaiting requested review from sergey-semenov

@elizabethandrews elizabethandrews Awaiting requested review from elizabethandrews

@Fznamznon Fznamznon Awaiting requested review from Fznamznon

@premanandrao premanandrao Awaiting requested review from premanandrao

@erichkeane erichkeane Awaiting requested review from erichkeane

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vzakhari @bader @jinge90 @againull @pvchupin