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.

Diff Detail

Repository
rL LLVM

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 ↗(On Diff #211472)

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

clang/include/clang/Driver/Options.td
1759–1761 ↗(On Diff #211472)

While there add description to ftime_report and document their difference?

llvm/lib/Support/TimeProfiler.cpp
27–30 ↗(On Diff #211472)

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 ↗(On Diff #211472)

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 ↗(On Diff #211472)

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 ↗(On Diff #211472)

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

28 ↗(On Diff #211472)

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 ↗(On Diff #211472)

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

llvm/lib/Support/TimeProfiler.cpp
27–30 ↗(On Diff #211472)

Sure, thanks.

28 ↗(On Diff #211472)

Moved to struct TimeProfiler member.

28 ↗(On Diff #211472)

Hmm, ok, removing default value.

Looks ok

clang/docs/ClangCommandLineReference.rst
1943–1947 ↗(On Diff #211472)

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 ↗(On Diff #211472)

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 ↗(On Diff #211472)

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 ↗(On Diff #211472)

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.