[flang-commits] [PATCH] D104019: [flang] Add initial implementation for CPU_TIME

[Diana Picus via Phabricator via flang-commits]

rovka added a comment.

In D104019#2826366 <https://reviews.llvm.org/D104019#2826366>, @rovka wrote:

> In D104019#2825717 <https://reviews.llvm.org/D104019#2825717>, @Meinersbur wrote:
>
>> This fails with msvc <http://meinersbur.de:8011/#/builders/146/builds/699> which apparently optimized the loop away:
>>
>>   [ RUN      ] TimeIntrinsics.CpuTime
>>   C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\flang\unittests\RuntimeGTest\Time.cpp(34): error: Expected: (end) > (start), actual: 2.000000e-03 vs 2.000000e-03
>>   [  FAILED  ] TimeIntrinsics.CpuTime (1 ms)
>>
>> I think assuming how long it takes to complete a volatile write is fragile.
>>
>> Instead I suggest to test CpuTime by repeatedly calling it until it changes.
>
> Hmm, that's probably better - if it never changes then we'll timeout eventually, right? I'll try to commit that today.

But just for the record, are you sure that the loop is optimized away? Have you checked the assembly? I'm thinking there might be an issue with the resolution of the timer used on Windows - we might need a better implementation for it than the default (just like I added for POSIX systems).

It would be nice if our test could flag when the resolution of the timer isn't good enough on a given implementation, for some definition of "good enough". Naturally, one can expect that the definition of "good enough" will change in the future, and our test can change with it. Ideally, the test would be representative of the amount of work that we expect to be able to time. OTOH I don't know if this test is the place for that, or if we should just add some microbenchmarks to the test-suite.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104019/new/

https://reviews.llvm.org/D104019