-
Notifications
You must be signed in to change notification settings - Fork 769
[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.
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] Make half default constructor constexpr #8067
Conversation
Unfortunately, it seem to be impossible to have a trivial default constexpr constructor until we enable C++23 mode. Therefore, this patch made a few other adjustments to account for non-trivial default constructor of `half` class.
static_assert(std::is_default_constructible<To>::value, | ||
"To must be default constructible"); |
There was a problem hiding this comment.
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
@@ -137,7 +137,8 @@ namespace host_half_impl { | |||
// The main host half class | |||
class __SYCL_EXPORT half { | |||
public: | |||
half() = default; | |||
constexpr half() = default; |
There was a problem hiding this comment.
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 classT
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
Aconstexpr
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 requiresTo
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
intounion
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
containingsycl::half
or provide default member initializer forsycl::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 havedevice_global<sycl::half>
:static_assert(std::is_trivially_default_constructible<T>::value,
Superseded by #8503 |
Unfortunately, it seem to be impossible to have a trivial default constexpr constructor until we enable C++23 mode. Therefore, this patch made a few other adjustments to account for non-trivial default constructor of
half
class.