-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[flang-rt] Runtime implementation of extended intrinsic function SECNDS() #152021
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
base: main
Are you sure you want to change the base?
Conversation
flang-rt/lib/runtime/command.cpp
Outdated
@@ -309,6 +310,12 @@ std::int32_t RTNAME(Hostnm)( | |||
return status; | |||
} | |||
|
|||
float RTNAME(Secnds)(float *refTime, const char *sourceFile, int line) { |
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 there a risk that lowering will emit a call with a null dummy argument pointer?
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.
Lowering, probably not. So just not have this Secnds
wrapper at all? Have lowering go directly to secnds_
?
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.
If there are other examples in lowering that work in that way, sure. Otherwise, a trivial wrapper is okay.
flang-rt/lib/runtime/extensions.cpp
Outdated
float FORTRAN_PROCEDURE_NAME(secnds)(float *refTime) { | ||
constexpr float FAIL_SECNDS{-1.0f}; // Failure code for this function | ||
// Failure code for time functions that return std::time_t | ||
constexpr time_t FAIL_TIME{std::time_t{-1}}; |
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.
maybe this should be std::time_t
flang-rt/lib/runtime/extensions.cpp
Outdated
// comes out to about 194 days. Thus, need to pick a starting point. | ||
// Given the description of this function, midnight of the current | ||
// day is the best starting point. | ||
static time_t startingPoint{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.
not thread-safe
should be std::time_t
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.
not thread-safe
Definitely not. Would C++ thread_local
work in this context? Or would it only work for threads created by C++ runtime?
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.
Or dodge the problem entirely by not saving midnight, just calculating it when needed.
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.
Here's the problem, as I see it: (1) we need to keep returned values low to not overwhelm the limited precision of float
and (2) the user can call secnds()
days apart for the same process runtime, so can't always go back to "this day's midnight".
Hmm, I suppose I could pre-compute "midnight" when flang-rt initializes. Is there an init function suitable for this?
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.
Please don't add overhead to every program.
If the user passes in a non-zero reference time, you have to use it without question, yes? If the user passes in a zero reference time, it means the most recent midnight, yes?
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.
If the user passes in a non-zero reference time, you have to use it without question, yes?
I can't use it without question, because I created it in the first place, so I know its limitations.
Let's say the user initially called s1=secnds(0.0)
on Day 1 at 5 p.m., so s1
is seconds between 0:00 and 17:00. Then on Day 3 the user called s2=secnds(s1)
at 10 a.m. If I just blindly use s2 - s1
, the result would be wrong. I need to store something to compensate for loss of information.
I think I can use C++ atomic exchange operations to make this safer.
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.
Be sure to not create a dependence on the C++ runtime library binaries from libflang_rt.runtime.
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.
Yeah, I'll check if new dependencies showed up.
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.
Dependencies before and after using atomics seem to be the same:
$ ldd ./clang/22/lib/x86_64-unknown-linux-gnu/libflang_rt.runtime.so
linux-vdso.so.1 (0x00007fff0eff4000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f03e025d000)
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f03e0176000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f03dff4d000)
/lib64/ld-linux-x86-64.so.2 (0x00007f03e1065000)
Not sure about Windows.
flang-rt/lib/runtime/command.cpp
Outdated
@@ -309,6 +310,12 @@ std::int32_t RTNAME(Hostnm)( | |||
return status; | |||
} | |||
|
|||
float RTNAME(Secnds)(float *refTime, const char *sourceFile, int line) { |
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 assume the separation of Secnds
to lower case secnds
is to allow you to test it in this PR. Command.h seems to be runtime functions related to the command line, so I think extenssions.cpp/h is the right place for this not command.h. Maybe add the argument checking back to the implementation in extenssions.cpp and test the function in a unit test?
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 was assuming that the other entry point was to handle calls that were to the non-intrinsic external name, as in
EXTERNAL SECNDS
PRINT *, SECNDS(0.)
END
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 was assuming that the other entry point was to handle calls that were to the non-intrinsic external name
Yes.
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.
Regarding Runtime/Command.h
, it seems to have functions not just related to command line.
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.
Ok, if we think providing the users both the external function implementation and the intrinsic itself, another alternative would be to put the two function functions next to each other in extension.cpp
. Command.cpp still seems like the wrong place for this runtime function.
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.
Regarding Runtime/Command.h, it seems to have functions not just related to command line.
Yeah I am pretty sure I added one of those functions without anyone telling me this was the wrong place for that function. Just because other people have made the mistake doesn't mean we should keep doing it.
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 don't mind moving things, I just want to make sure things are moved to the right locations. :-)
To me, all these "command" sources seemed more like "runtime commands", not necessarily command line. Here's my understanding of these files:
flang/include/flang/Optimizer/Builder/Runtime/Command.h
- MLIR building routines for runtime operations ("runtime commands" perhaps?)flang/include/flang/Runtime/command.h
- was this intended for command line argument handling only or was this intended for "runtime operations/commands" callable for MLIR building routines in aboveBuilder/Runtime/Command.h
? Right now it seems to contain all_FortranA...
runtime routines callable by lowering.flang-rt/lib/runtime/command.cpp
- currently it's implementation of_FortranA...
runtime routines declared inflang/include/flang/Runtime/command.h
above.flang/include/flang/Runtime/extensions.h
- declarations for all the extensions, suitable forexternal
usage.flang-rt/lib/runtime/extensions.cpp
- implementations for all the extensions.
So the question is whether the wrapper _FortranA...
routines for the extensions should move to extensions.cpp
. It seems to me that they should be together with the other wrapper routines in flang-rt/lib/runtime/command.cpp
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.
command.h/.cpp
should only contain things related to the command-line intrinsics. It accumulated more, but certainly not all of the RTNAME(...)
entry points, probably due to initial laziness and later following of poor examples. The miscellaneous stuff like HostNm
really shouldn't be in there.
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.
Ok, I shall move secnds()
stull to extensions.cpp, as @akuhlens suggested.
Andre, since you had the right idea, perhaps you want to do a general clean-up patch to move things to the right places?
return static_cast<float>(diffStartingPoint) - *refTime; | ||
} | ||
|
||
float RTNAME(Secnds)(float *refTime, const char *sourceFile, int line) { |
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.
Moved _FortranA...
function to here.
flang-rt/lib/runtime/extensions.cpp
Outdated
static std::atomic<std::time_t> startingPoint{TIME_UNINITIALIZED}; | ||
std::time_t expected{TIME_UNINITIALIZED}; | ||
std::time_t localStartingPoint{TIME_UNINITIALIZED}; | ||
if (startingPoint.compare_exchange_strong(expected, TIME_INITIALIZING)) { |
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.
Attempt to support multiple threads. Only tested single threaded for now. Will test with OpenMP threading.
Until the compiler part is fully hooked up via #151878, tested this using
external
: