-
Notifications
You must be signed in to change notification settings - Fork 920
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
[REVIEW] duration type unary casts and binary ops #5394
Changes from 54 commits
2053a09
be19e6b
b9b3c39
4f9a5fa
989fe6c
97860f0
ab108a7
49cc59f
8fca5d5
805f538
b7c2781
aac5ebb
4f15a5a
1d72ee4
d398736
3c57ab9
1c9323a
3659135
26efd47
63a5d53
81a872f
6dc6b89
220b4b8
f596e55
1bd2c5a
9dbd5cd
de7e262
5ee73a0
05d6571
bfcec07
ce374cb
57af9ea
8bdceab
e5d55b9
b7d580a
3fee810
c7b8778
6969939
04029c9
c5e6f02
65544bd
ad8df2e
35df63a
3bc0c64
d16a95b
803e1cc
bed8374
0449a2a
240041f
61e2101
73c1221
3021455
4207233
43cff5e
183c95b
0b5b6a4
a5b7363
2732be4
83e2a35
16bdb08
b78a57c
cfe0b95
1ae05be
351b7f3
8d760e9
a5062e2
0d2b0d2
2408894
a93f300
e180913
b936244
332e2d0
dc9ec82
75ed6fe
d9ee191
6c57c59
ae0a31d
b7de3c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,23 +32,26 @@ namespace cudf { | |
namespace detail { | ||
// TODO: Use chrono::utc_clock when available in libcu++? | ||
template <class Duration> | ||
using time_point = simt::std::chrono::time_point<simt::std::chrono::system_clock, Duration>; | ||
using time_point = simt::std::chrono::sys_time<Duration>; | ||
|
||
template <class Duration> | ||
struct timestamp : time_point<Duration> { | ||
// Bring over base class constructors and make them visible here | ||
sriramch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
using time_point<Duration>::time_point; | ||
|
||
// This is needed as __shared__ objects of this type can't be assigned in device code | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is only a problem with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this also shows up with |
||
// when the initializer list constructs subobjects with values, which is what std::time_point | ||
// does. | ||
constexpr timestamp() : time_point<Duration>(Duration()){}; | ||
constexpr timestamp(Duration d) : time_point<Duration>(d){}; | ||
|
||
// Implicitly convert a tick count into a timestamp | ||
// TODO: is this still needed and is this operation even meaningful? The duration units | ||
// cannot be inferred from this | ||
sriramch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
constexpr timestamp(typename Duration::rep r) : time_point<Duration>(Duration(r)){}; | ||
/** | ||
* @brief Constructs a new timestamp by copying the contents of another | ||
* `time_point` and converting its duration value if necessary. | ||
* | ||
* @param other The `timestamp` to copy | ||
*/ | ||
// TODO: This explict truncation is intended? | ||
template <class FromDuration> | ||
inline constexpr explicit timestamp(time_point<FromDuration> const& other) | ||
: time_point<Duration>(simt::std::chrono::duration_cast<Duration>(other.time_since_epoch())){}; | ||
|
||
// The inherited copy constructor will hide the auto generated copy constructor; | ||
// hence, explicitly define and delegate | ||
constexpr timestamp(const time_point<Duration>& other) : time_point<Duration>(other) {} | ||
}; | ||
} // namespace detail | ||
|
||
|
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.
i'll remove this before the merge; i'm keeping this only to get the build pipeline to pass.
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.
Is this left in so the CUDA 10.0 CI job passes? Are we expecting to remove CUDA 10.0 support before merging this PR? If not, should we guard this definition with an
#ifdef
so it's only include when the CUDA version is strictly 10.0?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.
i have now guarded this such that this specialization is applicable only for 10.0 builds.
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.
I would not take this out until we are well past 10.0, just in case someone chooses to keep using CUDA 10.0 "at their own risk". It's one thing to not support a CUDA version, but to knowingly block it is another...
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.
It is undefined behavior to include this specialization. We shouldn't be including it in a released build whatsoever.
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.
there is a guard to enable this only for cuda 10.0. if this is removed, then 10.0 ci builds will fail. i'll remove this if 10.0 support is removed from ci.
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.
it looks like we are no longer building on 10.0. i'll remove this code block and check to be sure.