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

Compatibility: Only use sycl::half if SYCL_CTS_ENABLE_HALF_TESTS is set #872

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Feb 17, 2024

This PR improves compatibility with implementations that do not expose the sycl::half type (notably, SimSYCL built with GCC).

There might be additional usages I missed, in tests that SimSYCL is currently unable to compile for other reasons.

Question to the reviewers: Is it necessary to exclude some translation units in CMake when the file names are *fp16.cpp or are these handled automatically?

util/math_reference.h Outdated Show resolved Hide resolved
util/type_traits.h Outdated Show resolved Hide resolved
@AlexeySachkov
Copy link
Contributor

Question to the reviewers: Is it necessary to exclude some translation units in CMake when the file names are *fp16.cpp or are these handled automatically?

From what I see, they are filtered out automatically:

if(NOT SYCL_CTS_ENABLE_HALF_TESTS)
list(FILTER test_cases_list EXCLUDE REGEX .*_fp16\\.cpp$)
endif()

@keryell
Copy link
Member

keryell commented Feb 22, 2024

g++-13 implements already https://en.cppreference.com/w/cpp/types/floating-point from C++23. @fknorr Go for it! :-)

@keryell
Copy link
Member

keryell commented Jun 13, 2024

@fknorr Some merge from upstream?

@fknorr
Copy link
Contributor Author

fknorr commented Jun 13, 2024

I was not aware of the _Float16 support in newer GCC - this PR might not be required anymore for SimSYCL, I can check after merging #870 .

@keryell
Copy link
Member

keryell commented Sep 25, 2024

@fknorr #870 has been merged and Ubuntu 24.04 comes with g++-14.
The more we wait, the more the features. 😸

@fknorr
Copy link
Contributor Author

fknorr commented Dec 17, 2024

Sorry for abandoning this for so long!

I've rebased on top of #874 to avoid future merge conflicts.

@keryell unfortunately we still need the #ifdef chain since the CTS uses cmath functions (std::signbit etc) on all scalar float types, and these are not implemented for _Float16, at least not with libstdc++. We could get around this by raising the minimum C++ standard to 23 for SimSYCL (because 23 has std::float16_t including math builtins), but I'd rather not do this for compatibility reasons.

Edit: DPC++ passes these - but how?

@fknorr
Copy link
Contributor Author

fknorr commented Dec 26, 2024

@bader's suggestion on reordering the functions in math_reference.h turned out to be more painful than expected, since moving function prototypes around messed with overload resolution in DPC++ (not sure if the code was actually incorrect or if I was triggering a compiler bug).

I've gone ahead and re-grouped all definitions within the file according to feature macros:

  1. All scalar float and int functions
  2. Scalar half functions (#if SYCL_CTS_ENABLE_HALF_TESTS)
  3. Scalar double functions (#if SYCL_CTS_ENABLE_DOUBLE_TESTS)
  4. All vec overloads (referencing scalar versions)
  5. All marray overloads (#if !SYCL_CTS_COMPILING_WITH_ADAPTIVECPP)
  6. Overloads generic over both scalar type and dimensionality.

A few functions (like cross) did not fit the schema above, so I kept those with their scalar cousins.

cmake/FindDPCPP.cmake Outdated Show resolved Hide resolved
tests/marray_basic/marray_operators.h Show resolved Hide resolved
tests/common/common_vec.h Outdated Show resolved Hide resolved
util/math_reference.h Outdated Show resolved Hide resolved
@fknorr fknorr force-pushed the compat-half branch 3 times, most recently from 7840ed6 to 0070e56 Compare December 27, 2024 13:04
tests/common/common_vec.h Outdated Show resolved Hide resolved
tests/common/common_vec.h Outdated Show resolved Hide resolved
tests/common/common_vec.h Outdated Show resolved Hide resolved
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