This is an archive of the discontinued LLVM Phabricator instance.

[flang] Add POSIX implementation for CPU_TIME
ClosedPublic

Authored by rovka on Jun 10 2021, 3:55 AM.

Details

Summary

Add an implementation for CPU_TIME using the POSIX function
clock_gettime. I think on most POSIX systems this will be included for
free via <ctime>, which corresponds to "time.h" (YMMV, we can fix the
code if the need arises).

Detecting that clock_gettime is available is tricky. For instance, commit
827407a86aa07 used the following incantation in f18-parse-demo.cpp:

#if _POSIX_C_SOURCE >= 199309L && _POSIX_TIMERS > 0 && _POSIX_CPUTIME && \

defined CLOCK_PROCESS_CPUTIME_ID

This doesn't work on my AArch64 Ubuntu system, which provides
clock_gettime but doesn't define _POSIX_TIMERS. Since finding the right
combination of macros requires infinite time, patience and access to
sundry POSIX systems, we should probably try a different approach.

This patch attempts to use SFINAE instead of the preprocessor to choose
an implementation for CPU_TIME. We define a helper function template
which lets us check if clock_gettime is available (and has the
interface we expect). I hope the comments explain it well enough.

This approach has the advantage that it keeps the detection of
clock_gettime close to the code that uses it. An alternative would be to
use CMake to check for the symbol (not sure which approach is likely to
run into more quirks).

Looking forward to feedback!

Diff Detail

Event Timeline

rovka created this revision.Jun 10 2021, 3:55 AM
rovka requested review of this revision.Jun 10 2021, 3:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2021, 3:55 AM
klausler accepted this revision.Jun 10 2021, 10:15 AM

Thanks for figuring this out.

flang/runtime/time-intrinsic.cpp
64

We've been using braces on all if statements in f18.

This revision is now accepted and ready to land.Jun 10 2021, 10:15 AM
rovka updated this revision to Diff 351359.Jun 11 2021, 1:13 AM

Added braces around if. NFCI.

rovka added a comment.Jun 11 2021, 1:14 AM

Thanks for figuring this out.

Thanks for the review :)
Can you also have a look at https://reviews.llvm.org/D104019 ? It needs to go in first.

This revision was automatically updated to reflect the committed changes.