This is an archive of the discontinued LLVM Phabricator instance.

[flang] More work on SYSTEM_CLOCK runtime API and implementation
ClosedPublic

Authored by klausler on Oct 6 2021, 4:42 PM.

Details

Summary

To get proper wrap-around behavior for the various kind parameter
values of the optional COUNT= and COUNT_MAX= dummy arguments to
the intrinsic subroutine SYSTEM_CLOCK, add an extra argument to
the APIs for lowering to pass the integer kind of the actual argument.
Avoid confusion by requiring that both actual arguments have the same
kind when both are present. The results of the runtime functions
remain std::int64_t and lowering should still convert them before
storing to the actual argument variables.

Rework the implementation a bit to accomodate the dynamic
specification of the kind parameter, and to clean up some coding
issues with preprocessing and templates.

Use the kind of the COUNT=/COUNT_MAX= actual arguments to determine
the clock's resolution, where possible, in conformance with other
Fortran implementations.

Diff Detail

Event Timeline

klausler created this revision.Oct 6 2021, 4:42 PM
klausler requested review of this revision.Oct 6 2021, 4:42 PM
rovka added inline comments.Oct 7 2021, 4:08 AM
flang/runtime/time-intrinsic.cpp
125

Isn't maxCount actually GetHUGE - 1? (Since you're doing % at the end of GetSystemClockCount).

161

Should we extract this into a helper, so it stays in sync with GetSystemClockCountMax?

flang/unittests/Runtime/Time.cpp
71

Maybe also add tests for kind = 16?

klausler marked 3 inline comments as done.Oct 7 2021, 11:16 AM
klausler added inline comments.
flang/runtime/time-intrinsic.cpp
125

Thanks for noticing that!

161

Sure, will do.

flang/unittests/Runtime/Time.cpp
71

Good idea, will do.

klausler updated this revision to Diff 377989.Oct 7 2021, 1:26 PM
klausler marked 3 inline comments as done.

Address all review comments.

rovka accepted this revision.Oct 8 2021, 1:58 AM

LGTM, thanks.

This revision is now accepted and ready to land.Oct 8 2021, 1:58 AM
rovka added inline comments.Oct 8 2021, 2:18 AM
flang/runtime/time-intrinsic.cpp
78

Actually I think this one needs -1 too :)

klausler added inline comments.Oct 8 2021, 10:10 AM
flang/runtime/time-intrinsic.cpp
78

I don't understand. The math seems straightforward and correct to me.

rovka added inline comments.Oct 11 2021, 1:53 AM
flang/runtime/time-intrinsic.cpp
78

Right, this comment was supposed to be for line 125. I think Phabricator got confused because I commented from the Diff 1 vs Diff 2 view, sorry about that.

125

I meant here: you updated the preferred_implementation with -1, but not the fallback_implementation. They both do % GetHUGE, so they should both have -1.

klausler updated this revision to Diff 378711.Oct 11 2021, 9:56 AM

Fix arithmetic edge cases.

rovka added a comment.Oct 12 2021, 1:33 AM

Ok, I think you can land this now :)

Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2021, 9:24 AM
brooks added a subscriber: brooks.Oct 12 2021, 11:33 AM
brooks added inline comments.
flang/include/flang/Runtime/time-intrinsic.h
16

There needs to be an include that defined std::size_t here. This currently fails to compile on FreeBSD.

klausler added inline comments.Oct 12 2021, 11:57 AM
flang/include/flang/Runtime/time-intrinsic.h
16

Will do.

Fixed in llvm-project/main; thanks for the notification.