Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
68a1a81
[flang-rt] Runtime implementation of extended intrinsic function SECN…
eugeneepshteyn Aug 4, 2025
716f82d
clang-format
eugeneepshteyn Aug 4, 2025
8bbe4eb
Fixed a typo
eugeneepshteyn Aug 4, 2025
5f671e7
Fixed init form
eugeneepshteyn Aug 4, 2025
9c53878
The failure code should be negative
eugeneepshteyn Aug 4, 2025
80b61ed
Introduce FAIL_TIME
eugeneepshteyn Aug 4, 2025
26b1265
Merge branch 'llvm:main' into secnds-runtime
eugeneepshteyn Aug 4, 2025
78a8d07
Code review: moved RTNAME(Secnds) to extensions.cpp. Added missing t…
eugeneepshteyn Aug 5, 2025
2f3a274
Attempt at using atomic operations in secnds_() to make it reentrant
eugeneepshteyn Aug 5, 2025
9940825
clang-format
eugeneepshteyn Aug 5, 2025
7268c8c
Fixed init
eugeneepshteyn Aug 5, 2025
812760f
Merge branch 'llvm:main' into secnds-runtime
eugeneepshteyn Aug 5, 2025
205818e
Reset startingPoint to 'uninitialized' on failure
eugeneepshteyn Aug 5, 2025
5945264
Changed currentStartingPoint initialization to use loop in all cases
eugeneepshteyn Aug 5, 2025
5cdb74d
clang-format
eugeneepshteyn Aug 5, 2025
599713e
Code review feedback
eugeneepshteyn Aug 5, 2025
4ae3cd3
clang-format
eugeneepshteyn Aug 5, 2025
fdd310a
Moved RTNAME(Secnds) declaration to extensions.h
eugeneepshteyn Aug 5, 2025
1cf3a46
Simplified the implementation based on Peter's suggestion
eugeneepshteyn Aug 6, 2025
aa3d3d4
clang-format
eugeneepshteyn Aug 6, 2025
0603b56
Merge branch 'llvm:main' into secnds-runtime
eugeneepshteyn Aug 6, 2025
fb75bee
Extracted common implementation into SecndsImpl(), which can then be …
eugeneepshteyn Aug 6, 2025
27ddc47
clang-format
eugeneepshteyn Aug 6, 2025
a61d035
Merge branch 'main' into secnds-runtime
eugeneepshteyn Aug 6, 2025
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
7 changes: 7 additions & 0 deletions flang-rt/lib/runtime/command.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "flang-rt/runtime/stat.h"
#include "flang-rt/runtime/terminator.h"
#include "flang-rt/runtime/tools.h"
#include "flang/Runtime/extensions.h"
#include <cerrno>
#include <cstdlib>
#include <limits>
Expand Down Expand Up @@ -309,6 +310,12 @@ std::int32_t RTNAME(Hostnm)(
return status;
}

float RTNAME(Secnds)(float *refTime, const char *sourceFile, int line) {
Copy link
Contributor

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?

Copy link
Contributor Author

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_?

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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 above Builder/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 in flang/include/flang/Runtime/command.h above.
  • flang/include/flang/Runtime/extensions.h - declarations for all the extensions, suitable for external 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

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Terminator terminator{sourceFile, line};
RUNTIME_CHECK(terminator, refTime != nullptr);
return FORTRAN_PROCEDURE_NAME(secnds)(refTime);
}

std::int32_t RTNAME(PutEnv)(
const char *str, size_t str_length, const char *sourceFile, int line) {
Terminator terminator{sourceFile, line};
Expand Down
41 changes: 41 additions & 0 deletions flang-rt/lib/runtime/extensions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,47 @@ void FORTRAN_PROCEDURE_NAME(qsort)(int *array, int *len, int *isize,
// PERROR(STRING)
void RTNAME(Perror)(const char *str) { perror(str); }

// GNU extension function SECNDS(refTime)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is ready for review. I did some testing with multiple threads. Test program:

use omp_lib
external secnds
real sec_mid
real results(1000)
integer i, num_threads

results = 0.0
!$omp parallel do
do i=1, size(results)
  results(i) = secnds(0.0)
  num_threads = omp_get_num_threads()
end do
!$omp end parallel do

print *, "Number of threads in parallel region:", num_threads

sec_mid = minval(results)
print *, "Seconds from midnight: min:", sec_mid, ", max:", maxval(results) 

call sleep(2)

!$omp parallel do
do i=1, size(results)
  results(i) = secnds(sec_mid)
end do
!$omp end parallel do

print *, "Seconds from sec_mid: min:", minval(results), ", max:", maxval(results)
print *, "Seconds from midnight:", secnds(0.0)
end

Output:

$ ./a.out 
 Number of threads in parallel region: 256
 Seconds from midnight: min: 46489. , max: 46489.
 Seconds from sec_mid: min: 2. , max: 2.
 Seconds from midnight: 46491.

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}};
Copy link
Contributor

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

if (!refTime) {
return FAIL_SECNDS;
}
std::time_t now{std::time(nullptr)};
if (now == FAIL_TIME) {
return FAIL_SECNDS;
}
// In float result, we can only precisely store 2^24 seconds, which
// 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};
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor Author

@eugeneepshteyn eugeneepshteyn Aug 4, 2025

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

if (!startingPoint) {
struct tm timeInfo;
#ifdef _WIN32
if (localtime_s(&timeInfo, &now)) {
return FAIL_SECNDS;
}
#else
if (!localtime_r(&now, &timeInfo)) {
return FAIL_SECNDS;
}
#endif
// Back to midnight
timeInfo.tm_hour = 0;
timeInfo.tm_min = 0;
timeInfo.tm_sec = 0;
startingPoint = std::mktime(&timeInfo);
if (startingPoint == FAIL_TIME) {
return FAIL_SECNDS;
}
}
double diffStartingPoint{std::difftime(now, startingPoint)};
return static_cast<float>(diffStartingPoint) - *refTime;
}

// GNU extension function TIME()
std::int64_t RTNAME(time)() { return time(nullptr); }

Expand Down
2 changes: 2 additions & 0 deletions flang/include/flang/Runtime/command.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ std::int32_t RTNAME(Hostnm)(
std::int32_t RTNAME(PutEnv)(
const char *str, size_t str_length, const char *sourceFile, int line);

float RTNAME(Secnds)(float *refTime, const char *sourceFile, int line);

// Calls unlink()
std::int32_t RTNAME(Unlink)(
const char *path, size_t pathLength, const char *sourceFile, int line);
Expand Down
3 changes: 3 additions & 0 deletions flang/include/flang/Runtime/extensions.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,5 +90,8 @@ void RTNAME(Perror)(const char *str);
// MCLOCK -- returns accumulated time in ticks
int FORTRAN_PROCEDURE_NAME(mclock)();

// GNU extension subroutine SECNDS(refTime)
float FORTRAN_PROCEDURE_NAME(secnds)(float *refTime);

} // extern "C"
#endif // FORTRAN_RUNTIME_EXTENSIONS_H_