This is an archive of the discontinued LLVM Phabricator instance.

[Support][Test] Time profiler: add regression test
ClosedPublic

Authored by anton-afanasyev on May 14 2019, 1:04 PM.

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 14 2019, 1:04 PM

Could you please move the test to a more approriate location? (ie. clang/trunk/test/Driver/)

clang/tools/driver/cc1_main.cpp
245

This seems a bit too chatty. Suround these two lines with if (Config->Verbose) ?

llvm/test/Support/check-time-trace.cxx
4

I don't see any other -ftime-trace tests, I would add a few more exhaustive file format checks here.

anton-afanasyev marked 4 inline comments as done.Jun 7 2019, 4:12 AM

Could you please move the test to a more approriate location? (ie. clang/trunk/test/Driver/)

Thanks, I've moved it there.

clang/tools/driver/cc1_main.cpp
245

I don't think it should be done this way for several reasons:

  1. This info is actually needed by user when one uses --ftime-trace since json-file is usually dumped to random filename located in /tmp/. Therefore --ftime-trace means --ftime-trace --verbose itself. Without this info user cannot understand where dumped file is. How can he know about --verbose?
  2. Option --ftime-report is much more chatty, it dumps several tables to terminal. This option just dumps to file, but tells where file is.
  3. I have not found elegant way to check --verbose option here, one should add it to clang::FrontendOptions or so.
llvm/test/Support/check-time-trace.cxx
4

Ok, I've added more checks, parsing json file by python.

anton-afanasyev marked 2 inline comments as done.

Updated

aganea accepted this revision.Jun 7 2019, 5:48 AM

Thanks Anton!

This revision is now accepted and ready to land.Jun 7 2019, 5:48 AM
This revision was automatically updated to reflect the committed changes.
plotfi added a subscriber: plotfi.Jun 7 2019, 12:18 PM

cfe/trunk/test/Driver/check-time-trace.cpp appears to fail on Darwin. Did you mean to pass the target explicitly ?

plotfi added inline comments.Jun 7 2019, 12:24 PM
cfe/trunk/test/Driver/check-time-trace.cpp
1 ↗(On Diff #203544)

This test should probably have // REQUIRES: shell

anton-afanasyev marked an inline comment as done.Jun 7 2019, 2:00 PM

cfe/trunk/test/Driver/check-time-trace.cpp appears to fail on Darwin. Did you mean to pass the target explicitly ?

This commit is reverted now. How can I pass the target explicitly?

cfe/trunk/test/Driver/check-time-trace.cpp
1 ↗(On Diff #203544)

Thanks! I haven't found REQUIRES: shell here: https://llvm.org/docs/CommandGuide/FileCheck.html. Does this directive mean the target OS must have utils like awk and xargs?