Page MenuHomePhabricator

[Support] Fix `-ftime-trace-granularity` option
ClosedPublic

Authored by anton-afanasyev on Jul 24 2019, 5:15 AM.

Details

Summary

Move -ftime-trace-granularity option to frontend options. Without patch
this option is showed up in the help for any tool that links libSupport.

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 24 2019, 5:15 AM
lebedev.ri added inline comments.
clang/docs/ClangCommandLineReference.rst
1943–1947

While there add a description to -ftime-report and document how they are different?

clang/include/clang/Driver/Options.td
1759–1761

While there add description to ftime_report and document their difference?

llvm/lib/Support/TimeProfiler.cpp
27–30

Can TimeTraceGranularity perhaps be stored in TimeTraceProfilerInstance?

sammccall accepted this revision.Jul 24 2019, 5:27 AM
sammccall added inline comments.
clang/docs/ClangCommandLineReference.rst
1951

is there any possibility of wanting a granularity <1ms in the future? This sets up a backward-compatibility trap if so.

clang/lib/Frontend/CompilerInvocation.cpp
1772

You've got the default repeated here. Is it possible to set this conditionally here, or use the existing value as default like

Opts.TTG = getLastArgIntValue(Args, OPT_fttg_EQ, Opts.TTG, Diags)

llvm/lib/Support/TimeProfiler.cpp
28

why does this have a default value? It shouldn't be possible to use it without overwriting it, IIUC

28

static

This revision is now accepted and ready to land.Jul 24 2019, 5:27 AM
anton-afanasyev marked 8 inline comments as done.

Update

clang/docs/ClangCommandLineReference.rst
1951

Does it really make sense? One can use -ftime-trace-granularity=0, which can show all events.

llvm/lib/Support/TimeProfiler.cpp
27–30

Sure, thanks.

28

Moved to struct TimeProfiler member.

28

Hmm, ok, removing default value.

Looks ok

clang/docs/ClangCommandLineReference.rst
1943–1947

I mean, state that one is chrome json trace and another is console table output

anton-afanasyev marked an inline comment as done.Jul 24 2019, 6:25 AM
anton-afanasyev added a subscriber: aras-p.
anton-afanasyev added inline comments.
clang/docs/ClangCommandLineReference.rst
1943–1947

The difference is actually deeper at the moment. -ftime-report output is buggy, short and uninformative (as noted by @aras-p in his blog post http://aras-p.info/blog/2019/01/12/Investigating-compile-times-and-Clang-ftime-report/). I would like just to remove -ftime-report option as obsolete and unsupported.

anton-afanasyev marked 2 inline comments as done.Jul 24 2019, 6:36 AM
anton-afanasyev added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1772

Ok, done.

anton-afanasyev marked an inline comment as done.

Remove default repeat

anton-afanasyev marked an inline comment as done.Jul 24 2019, 7:15 AM
anton-afanasyev added inline comments.
clang/docs/ClangCommandLineReference.rst
1951

To be more correct, this granularity value (-ftime-trace-granularity=0) will work after this old patch https://reviews.llvm.org/D63325 is commited.

This revision was automatically updated to reflect the committed changes.