Page MenuHomePhabricator

Add a new clang option "-ftime-trace=<value>"
ClosedPublic

Authored by dongjunduo on Jun 17 2022, 6:07 AM.

Details

Summary

The time profiler traces the stages during the clang compile
process. Each compiling stage of a single source file
corresponds to a separately .json file which holds its
time tracing data. However, the .json files are stored in the
same path/directory as its corresponding stage's '-o' option.
For example, if we compile the "demo.cc" to "demo.o" with option
"-o /tmp/demo.o", the time trace data file path is "/tmp/demo.json".

A typical c++ project can contain multiple source files in different
path, but all the json files' paths can be a mess.

The option "-ftime-trace" allows you to specify where the json
files should be stored. This allows the users to place the time trace
data files of interest in the desired location for further data analysis.

Usage:

  • clang/clang++ -ftime-trace ...
  • clang/clang++ -ftime-trace=the-directory-you-want ...
  • clang/clang++ -ftime-trace=the-directory-you-want/ ...
  • clang/clang++ -ftime-trace=the-full-file-path-you-want ...

Diff Detail

Event Timeline

dongjunduo created this revision.Jun 17 2022, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2022, 6:07 AM
dongjunduo requested review of this revision.Jun 17 2022, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2022, 6:07 AM

Can you please add some test cases?

clang/tools/driver/cc1_main.cpp
260–267

What happens when TimeTracePath is not given? Ideally the originally behavior is not changed.

jamieschmeiser requested changes to this revision.Jun 17 2022, 10:50 AM

This is a good start. You should mention in the summary that when -c is not specified, the compiler is invoked with -o pointing at /tmp so the .json file will currently be placed there, which is confusing.

clang/include/clang/Driver/Options.td
2861

which specifies the output files for -ftime-trace

clang/tools/driver/cc1_main.cpp
262

What happens if the path specified does not exist? This needs to be handled (if it isn't) and needs a lit test for this situation also.

This revision now requires changes to proceed.Jun 17 2022, 10:50 AM

[Clang] Restore the old behaviors when "-ftime-trace-path" is not specified

dongjunduo marked an inline comment as done.

[Clang] change unclear help text pf "-ftime-trace-path"

Can you please add some test cases?

I have found that new-option unit tests are not included in clang/test or clang/unit-tests. Could u tell me where I should write the test cases?

clang/tools/driver/cc1_main.cpp
260–267

The originally behavior has been restored : )

Time-trace file "*.json" may be saved by following the origin logic if "-ftime-trace-path" is not be specified.

Whitney added inline comments.Jun 20 2022, 10:21 PM
clang/include/clang/Driver/Options.td
2861

What do you think of "which specifies the output directory for -ftime-trace"? As from your example, this option doesn't specify a file, instead it specifies a directory.

clang/include/clang/Frontend/FrontendOptions.h
502

Please update this comment according too.

[Clang] update help text of "ftime-trace-path"

Can you please use git rebase -i to collapse all the changes into a single change? If this isn't done, it is difficult to know what is being reviewed as the changes only show the differences since your last revision, not all of the changes.

Can you please use git rebase -i to collapse all the changes into a single change? If this isn't done, it is difficult to know what is being reviewed as the changes only show the differences since your last revision, not all of the changes.

I can see all the changes, make sure your link is not suffix with "new", or check the history tab to see what is it comparing.

dongjunduo marked 4 inline comments as done.Jun 22 2022, 6:46 AM

Can you please use git rebase -i to collapse all the changes into a single change? If this isn't done, it is difficult to know what is being reviewed as the changes only show the differences since your last revision, not all of the changes.

It seems that I have collapsed all the commits. The panel "Revision Contents=>Commits" shows only a single commit record. Is that what you mean? : )

Can you please use git rebase -i to collapse all the changes into a single change? If this isn't done, it is difficult to know what is being reviewed as the changes only show the differences since your last revision, not all of the changes.

I can see all the changes, make sure your link is not suffix with "new", or check the history tab to see what is it comparing.

It had the /new on the path. I removed it and now I can see everything. Thanks.

[Clang] change directory-store to path-store

Hi @jamieschmeiser @Whitney ,

I have changed the approach from directory-store to path-store, so that the user can specify the aim path to store the time trace json file.
If "-ftime-trace-path" is not specified, it will follow the default behavior.

In addition, I add a test-case to test the new option.

PTAL : )

dongjunduo edited the summary of this revision. (Show Details)Jun 23 2022, 7:30 AM
clang/test/Driver/check-time-trace-path.cpp
5 ↗(On Diff #439389)

This test is the same as the one in check-time-trace.cpp except for the path feature. A test file can have multiple tests in it. Rather than repeating all of the checks and source, it would be better to just put this test in the existing file. Just add lines 1-4 from this file into that file. If you need to special-case parts of it, you can use --check-prefix

dongjunduo updated this revision to Diff 439661.EditedJun 24 2022, 1:04 AM

[Clang] rewrite test case of "-ftime-trace-path"

@jamieschmeiser Done : )

You may add -ftime-trace=<file> instead of introducing a new spelling.

You may add -ftime-trace=<file> instead of introducing a new spelling.

I like this idea. There is an example of an optional argument on an option with -print-changed.

dongjunduo added inline comments.Jun 28 2022, 6:42 AM
clang/include/clang/Driver/Options.td
2848

@MaskRay @jamieschmeiser May I change the option "-ftime-trace" from a "Flag" type to a "Joined" type?

I have noticed that lld's option "-ftime-trace-file" is used to specify the output file name. It must be specified simultaneously with "-ftime-trace".

MaskRay added inline comments.Jun 28 2022, 1:03 PM
clang/include/clang/Driver/Options.td
2848

lld has removed --time-trace-file=. --time-trace= is the "Joined" option to specify the filename. clang -ftime-trace= LGTM.

[Clang] change "-ftime-trace-path" to "-ftime-trace"

Hi @jamieschmeiser @Whitney @MaskRay, I have changed "-ftime-trace-path" to "-ftime-trace". It is a well-spelling option name.

The user can turn on the time-trace by:

  • specifying "-ftime-trace" only and output it to the default directory (the same as the "-o" option's value)
  • specifying "-ftime-trace=the-directory-you-want"
  • specifying "-ftime-trace=the-directory-you-want/"
  • specifying "-ftime-trace=the-full-file-path-you-want"
Whitney added inline comments.Jun 30 2022, 6:29 AM
clang/test/Driver/check-time-trace.cpp
9

By default, the JSON file is stored in %T, as it is the directory for the output object file. Can you make a different directory, to ensure it is not just the default behavior?

[Clang] change test cases to different directory

dongjunduo marked an inline comment as done.Jun 30 2022, 6:47 PM
dongjunduo added inline comments.
clang/test/Driver/check-time-trace.cpp
9

Done. I have moved them to new directories for the next tests.

dongjunduo retitled this revision from Add a new clang option "-ftime-trace-path" to Add a new clang option "-ftime-trace".Jun 30 2022, 6:51 PM
dongjunduo edited the summary of this revision. (Show Details)
dongjunduo marked an inline comment as done.
dongjunduo retitled this revision from Add a new clang option "-ftime-trace" to Add a new clang option "-ftime-trace=<value>".Jun 30 2022, 6:53 PM
Whitney accepted this revision.Jun 30 2022, 6:54 PM
Whitney added inline comments.

[Clang] format the source file

dongjunduo marked an inline comment as done.Jun 30 2022, 7:00 PM
jamieschmeiser accepted this revision.Jul 11 2022, 8:20 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Jul 11 2022, 8:20 AM
MaskRay added inline comments.Jul 11 2022, 5:37 PM
clang/include/clang/Driver/Options.td
2860

ftime_trace_EQ

clang/tools/driver/cc1_main.cpp
263

The variable FileName can be removed

[Clang] restyle code

dongjunduo marked 2 inline comments as done.Jul 15 2022, 12:37 AM
MaskRay accepted this revision.Jul 15 2022, 1:29 AM

[Clang] fix mkdir error in ftime-trace=<value> test

This revision was automatically updated to reflect the committed changes.