Require allocated type to have only a trivial constructor for make_device_unique and make_host_unique #204
Conversation
…vice_unique and make_host_unique In principle we could call a non-trivial constructor in make_host_unique, but since the only point of the pinned host memory is to transfer data to/from device memory, one needs (well, almost) the same struct for the make_device_unique as well. I deviced to go for consistency.
|
I'm not super happy with the outcome, but I don't know if we can do much better (unless we continue to just ignore constructors in |
fwyzard
force-pushed
the
CMSSW_10_4_X_Patatrack
branch
from
6110cf4
to
b8334a2
Compare
| uint32_t const *clusInModule_ = nullptr; | ||
| uint32_t const *moduleId_ = nullptr; | ||
| uint32_t const *clusModuleStart_ = nullptr; | ||
| uint32_t const *moduleStart_; |
There was a problem hiding this comment.
why do you remove the nullptr initialisation ?
There was a problem hiding this comment.
Because those lead to constructor being non-trivial (since it would do some initialization).
There was a problem hiding this comment.
#include <iostream>
class Xyz {
int bar = 0;
};
int main(void) {
std::cout << "Xyz " << std::is_trivially_constructible_v<Xyz> << std::endl;
return 0;
}gives
Xyz 0
There was a problem hiding this comment.
Yes, of course sorry (trivial really means it does nothing...).
| @@ -72,6 +72,7 @@ class CUDAService { | |||
| template <typename T> | |||
| typename cudaserviceimpl::make_device_unique_selector<T>::non_array | |||
| make_device_unique(cuda::stream_t<>& stream) { | |||
| static_assert(std::is_trivially_constructible<T>::value, "Allocating with non-trivial constructor on the device memory is not supported"); | |||
There was a problem hiding this comment.
Would it make sense to fail the instantiation using std::enable_if_t<std::is_trivially_constructible<T>::value, typename ...> as the return type, instead of using a static_assert ?
Pro: the templates can be used with SFINAE.
Con: the error message may be harder to understand
There was a problem hiding this comment.
I'm probably not woken up enough, but what would be the benefit of SFINAE be with these?
There was a problem hiding this comment.
Right, no, I don't think there is any advantage...
I guess we could start playing with concepts (later).
|
And I do not understand the conflict, the branch does merge for me :-( |
|
No changes in performance (as expected) with respect to a67b49d: |
…vice_unique and make_host_unique (#204)
…vice_unique and make_host_unique (#204)
…vice_unique and make_host_unique (#204)
…vice_unique and make_host_unique (#204)
…vice_unique and make_host_unique (#204)
…vice_unique and make_host_unique (#204)
…vice_unique and make_host_unique (#204)
…vice_unique and make_host_unique (#204)
…vice_unique and make_host_unique (#204)
This PR addresses @fwyzard's comment #172 (comment) and adds a requirement for
Thave (only) trivial constructor inCUDAService::make_device_unique()andCUDAService::make_host_unique().In principle we could call a non-trivial constructor in make_host_unique, but since the only point of the pinned host memory is to transfer data to/from device memory, one needs (well, almost) the same struct for the make_device_unique as well. I deviced to go for consistency.