Skip to content

[SYCL] Make half default constructor constexpr #8067

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

Closed
wants to merge 2 commits into from
Closed

[SYCL] Make half default constructor constexpr #8067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f79639
[SYCL] Make half default constructor constexpr
AlexeySachkov Jan 20, 2023
cf8afc2
Mark half constructor as default
AlexeySachkov Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sycl/include/sycl/bit_cast.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ constexpr
#if __has_builtin(__builtin_bit_cast)
return __builtin_bit_cast(To, from);
#else // __has_builtin(__builtin_bit_cast)
static_assert(std::is_trivially_default_constructible<To>::value,
"To must be trivially default constructible");
static_assert(std::is_default_constructible<To>::value,
"To must be default constructible");
Comment on lines +44 to +45
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: I honestly don't know why would we need a trivial constructor here, but if necessary, we can return that check back and add a special case for half like we used to have, see 909a824

To to;
sycl::detail::memcpy(&to, &from, sizeof(To));
return to;
Expand Down
9 changes: 5 additions & 4 deletions sycl/include/sycl/half_type.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,8 @@ namespace host_half_impl {
// The main host half class
class __SYCL_EXPORT half {
public:
half() = default;
constexpr half() = default;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: this is a non-trivial constructor! std::is_trivially_default_constructible_v<sycl::half> is false.

Reason for that is because trivially constructible classes can't have non-static members with default initializers. From cppreference:

Trivial default constructor
The default constructor for class T is trivial (i.e. performs no action) if all of the following is true:

  • The constructor is not user-provided (i.e., is implicitly-defined or defaulted on its first declaration)
  • T has no non-static members with default initializers. (since C++11)

We have non-static member Buf with default initializer below and we can't remove it or otherwise we can't make the constructor constexpr.

Without default initializer for Buf, the following error will be emitted: defaulted definition of default constructor is not constexpr. The reason for that is because constexpr constructors must have all fields initialized: cppreference:

constexpr constructor
A constexpr constructor whose function body is not =delete; must satisfy the following additional requirements:

  • for the constructor of a class or struct, every base class sub-object and every non-variant non-static data member must be initialized.

Apparently, trivial default constexpr constructors are only available in C++23: P1331R2.

Not making the default constructor trivial brings several issues (of different impact):

  • sycl::bit_cast<sycl::half> doesn't work anymore, because it requires To type to be trivially default constructible. I don't see any reason for such strict requirement and therefore I'm relaxing it in this PR

  • putting sycl::half into union requires a bit more work from the user: from cppreference:

    If a union contains a non-static data member with a non-trivial default constructor, the default constructor of the union is deleted by default unless a variant member of the union has a default member initializer.

    That effectively means that user has to either provide default constructor for union containing sycl::half or provide default member initializer for sycl::half member to make it compile. You can find both approached used in this PR to adjust both implementation and tests.

  • device_global currently requires trivially default constructible types, i.e. it means we can't have device_global<sycl::half>:

    llvm/sycl/include/sycl/ext/oneapi/device_global/device_global.hpp

    Line 128 in 3d8a136

    static_assert(std::is_trivially_default_constructible<T>::value,


constexpr half(const half &) = default;
constexpr half(half &&) = default;

Expand Down Expand Up @@ -206,7 +207,7 @@ class __SYCL_EXPORT half {
friend class sycl::ext::intel::esimd::detail::WrapperElementTypeProxy;

private:
uint16_t Buf;
uint16_t Buf = {};
};

} // namespace host_half_impl
Expand Down Expand Up @@ -272,7 +273,7 @@ class half {
class [[__sycl_detail__::__uses_aspects__(aspect::fp16)]] half {
#endif
public:
half() = default;
constexpr half() = default;
constexpr half(const half &) = default;
constexpr half(half &&) = default;

Expand Down Expand Up @@ -548,7 +549,7 @@ class [[__sycl_detail__::__uses_aspects__(aspect::fp16)]] half {
friend class sycl::ext::intel::esimd::detail::WrapperElementTypeProxy;

private:
StorageT Data;
StorageT Data = {};
};
} // namespace half_impl

Expand Down
1 change: 1 addition & 0 deletions sycl/source/detail/builtins_relational.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ template <> union databitset<s::cl_double> {
template <> union databitset<s::cl_half> {
static_assert(sizeof(s::cl_short) == sizeof(s::cl_half),
"size of cl_half is not equal to 16 bits(cl_short).");
databitset(): i(0) {}
s::cl_half f;
s::cl_short i;
};
Expand Down
9 changes: 9 additions & 0 deletions sycl/test/basic_tests/half-constexpr-constructor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// RUN: %clangxx -fsycl -fsyntax-only %s

#include <sycl/sycl.hpp>

int main() {
constexpr sycl::half h;

return 0;
}
2 changes: 1 addition & 1 deletion sycl/test/regression/half_union.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

typedef union _u16_to_half {
unsigned short u;
sycl::half h;
sycl::half h = 0.0;
} u16_to_sycl_half;

int main() {
Expand Down