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

[dongjunduo]

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 ...

[Whitney]

Can you please add some test cases?

clang/tools/driver/cc1_main.cpp

258–263

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

[jamieschmeiser]

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

2832

which specifies the output files for -ftime-trace

clang/tools/driver/cc1_main.cpp

260

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.

[dongjunduo]

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

[dongjunduo]

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

[dongjunduo]

In D128048#3591829, @Whitney wrote:

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

258–263

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]

clang/include/clang/Driver/Options.td

2832

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.

[dongjunduo]

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

[jamieschmeiser]

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.

[Whitney]

In D128048#3601579, @jamieschmeiser wrote:

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]

In D128048#3601579, @jamieschmeiser wrote:

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? : )

[jamieschmeiser]

In D128048#3601588, @Whitney wrote:

In D128048#3601579, @jamieschmeiser wrote:

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.

[dongjunduo]

[Clang] change directory-store to path-store

[dongjunduo]

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 : )

[jamieschmeiser]

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]

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

@jamieschmeiser Done : )

[MaskRay]

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

[jamieschmeiser]

In D128048#3607295, @MaskRay wrote:

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]

clang/include/clang/Driver/Options.td

2819

@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]

clang/include/clang/Driver/Options.td

2819

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

[dongjunduo]

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

[dongjunduo]

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]

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?

[dongjunduo]

[Clang] change test cases to different directory

[dongjunduo]

clang/test/Driver/check-time-trace.cpp

9

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

[Whitney]

clang/tools/driver/cc1_main.cpp

263

No need braces for single instruction block. https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

[dongjunduo]

[Clang] format the source file

[jamieschmeiser]

LGTM. Thanks.

[MaskRay]

clang/include/clang/Driver/Options.td

2831

ftime_trace_EQ

clang/tools/driver/cc1_main.cpp

261

The variable FileName can be removed

[dongjunduo]

[Clang] restyle code

[dongjunduo]

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

clang/include/clang/Driver/Options.td

defm threadsafe_statics : BoolFOption<"threadsafe-statics",
   PosFlag<SetTrue>>;
 def ftime_report : Flag<["-"], "ftime-report">, Group<f_Group>, Flags<[CC1Option]>,
   MarshallingInfoFlag<CodeGenOpts<"TimePasses">>;
 def ftime_report_EQ: Joined<["-"], "ftime-report=">, Group<f_Group>,
   Flags<[CC1Option]>, Values<"per-pass,per-pass-run">,
   MarshallingInfoFlag<CodeGenOpts<"TimePassesPerRun">>,
   HelpText<"(For new pass manager) 'per-pass': one report for each pass; "
            "'per-pass-run': one report for each pass invocation">;
 def ftime_trace : Flag<["-"], "ftime-trace">, Group<f_Group>,
   HelpText<"Turn on time profiler. Generates JSON file based on output filename.">,
   DocBrief<[{
 Turn on time profiler. Generates JSON file based on output filename. Results
 can be analyzed with chrome://tracing or `Speedscope App
 <https://www.speedscope.app>`_ for flamegraph visualization.}]>,
   Flags<[CC1Option, CoreOption]>,
   MarshallingInfoFlag<FrontendOpts<"TimeTrace">>;
 def ftime_trace_granularity_EQ : Joined<["-"], "ftime-trace-granularity=">, Group<f_Group>,
   HelpText<"Minimum time granularity (in microseconds) traced by time profiler">,
   Flags<[CC1Option, CoreOption]>,
   MarshallingInfoInt<FrontendOpts<"TimeTraceGranularity">, "500u">;
+def ftime_trace_path : Joined<["-"], "ftime-trace-path=">, Group<f_Group>,
+  HelpText<"Path which stores the output files for -ftime-trace">,
+  Flags<[CC1Option, CoreOption]>,
+  MarshallingInfoString<FrontendOpts<"TimeTracePath">>;
 def fproc_stat_report : Joined<["-"], "fproc-stat-report">, Group<f_Group>,
   HelpText<"Print subprocess statistics">;
 def fproc_stat_report_EQ : Joined<["-"], "fproc-stat-report=">, Group<f_Group>,
   HelpText<"Save subprocess statistics to the given file">;
 def ftlsmodel_EQ : Joined<["-"], "ftls-model=">, Group<f_Group>, Flags<[CC1Option]>,
   Values<"global-dynamic,local-dynamic,initial-exec,local-exec">,
   NormalizedValuesScope<"CodeGenOptions">,
   NormalizedValues<["GeneralDynamicTLSModel", "LocalDynamicTLSModel", "InitialExecTLSModel", "LocalExecTLSModel"]>,

clang/include/clang/Frontend/FrontendOptions.h

public:
   Optional<std::vector<std::string>> AuxTargetFeatures;
 
   /// Filename to write statistics to.
   std::string StatsFile;
 
   /// Minimum time granularity (in microseconds) traced by time profiler.
   unsigned TimeTraceGranularity;
 
+  /// Path which stores the output files for -ftime-trace
+  std::string TimeTracePath;
+
 public:
   FrontendOptions()
       : DisableFree(false), RelocatablePCH(false), ShowHelp(false),
         ShowStats(false), TimeTrace(false), ShowVersion(false),
         FixWhatYouCan(false), FixOnlyWarnings(false), FixAndRecompile(false),
         FixToTemporaries(false), ARCMTMigrateEmitARCErrors(false),
         SkipFunctionBodies(false), UseGlobalModuleIndex(true),
         GenerateGlobalModuleIndex(true), ASTDumpDecls(false),

clang/lib/Driver/ToolChains/Clang.cpp

   Args.AddLastArg(CmdArgs, options::OPT_flax_vector_conversions_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_fobjc_sender_dependent_dispatch);
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_print_source_range_info);
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_parseable_fixits);
   Args.AddLastArg(CmdArgs, options::OPT_ftime_report);
   Args.AddLastArg(CmdArgs, options::OPT_ftime_report_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_ftime_trace);
   Args.AddLastArg(CmdArgs, options::OPT_ftime_trace_granularity_EQ);
+  Args.AddLastArg(CmdArgs, options::OPT_ftime_trace_path);
   Args.AddLastArg(CmdArgs, options::OPT_ftrapv);
   Args.AddLastArg(CmdArgs, options::OPT_malign_double);
   Args.AddLastArg(CmdArgs, options::OPT_fno_temp_file);
 
   if (Arg *A = Args.getLastArg(options::OPT_ftrapv_handler_EQ)) {
     CmdArgs.push_back("-ftrapv-handler");
     CmdArgs.push_back(A->getValue());
   }

clang/test/Driver/check-time-trace.cpp

 // RUN: %clangxx -S -ftime-trace -ftime-trace-granularity=0 -o %T/check-time-trace %s
 // RUN: cat %T/check-time-trace.json \
 // RUN:   | %python -c 'import json, sys; json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
 // RUN:   | FileCheck %s
+// RUN: %clangxx -S -ftime-trace -ftime-trace-path=%T/new-name.json -ftime-trace-granularity=0 -o %T/check-time-trace %s
+// RUN: cat %T/new-name.json \
+// RUN:   | %python -c 'import json, sys; json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
+// RUN:   | FileCheck %s
 
 // CHECK:      "beginningOfTime": {{[0-9]{16},}}
 // CHECK-NEXT: "traceEvents": [
 // CHECK:      "args":
 // CHECK:      "detail":
 // CHECK:      "dur":
 // CHECK:      "name":
 // CHECK-NEXT: "ph":
 // CHECK-NEXT: "pid":

clang/tools/driver/cc1_main.cpp

int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
 
   // If any timers were active but haven't been destroyed yet, print their
   // results now.  This happens in -disable-free mode.
   llvm::TimerGroup::printAll(llvm::errs());
   llvm::TimerGroup::clearAll();
 
   if (llvm::timeTraceProfilerEnabled()) {
     SmallString<128> Path(Clang->getFrontendOpts().OutputFile);
+    if (Clang->getFrontendOpts().TimeTracePath.empty()) {
+      // replace the suffix to '.json' directly
       llvm::sys::path::replace_extension(Path, "json");
+    } else {
+      Path.assign(Clang->getFrontendOpts().TimeTracePath);
+    }
     if (auto profilerOutput = Clang->createOutputFile(
             Path.str(), /*Binary=*/false, /*RemoveFileOnSignal=*/false,
             /*useTemporary=*/false)) {
       llvm::timeTraceProfilerWrite(*profilerOutput);
       profilerOutput.reset();
       llvm::timeTraceProfilerCleanup();
       Clang->clearOutputFiles(false);
     }