[PATCH] D133662: [Clang] Change -ftime-trace storing path and support multiple compilation jobs

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

Great.  Also, something you may want to consider, either as part of or after 
you land this code.  This really is a specific instance of a more generic 
problem: setting up option handling for something that saves results in a file 
for each compilation.  This is equally useful for other things such as listings 
and could also be used by something like print-changed (which currently just 
outputs to the stream), opt stats reporting, etc.  This code could be organized 
as a function (or possibly an object, depends...) that takes  a string for the 
extension, a lambda/template for the virtual call on whether to add the option 
to a tool so that off-handling, platform-isms, and where files are saved would 
all be captured neatly and would be re-usable.  InferTimeTrace and getPath, 
off-loading, platform-isms would be captured in a generic call that would look 
something like (in this instance)

  PerFileTraceGenerator(".json",
   [](Tool , Args )->bool{ return T->supportsTimeTrace() && 
Args.hasArg(options::OPT_ftime_trace, options::OPT_ftime_trace_EQ; },
   "-ftime-trace="); 

Each option that needs per file output would just call this function 
appropriately.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133662/new/

https://reviews.llvm.org/D133662

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133662: [Clang] Change -ftime-trace storing path and support multiple compilation jobs

[Mészáros Gergely via Phabricator via cfe-commits]

Maetveis added a comment.

In D133662#3798276 , @jamieschmeiser 
wrote:

> I had an email exchange with @dongjunduo about this situation.  He was a GSoC 
> student that @Whitney and I were mentoring for the past summer.  He agrees 
> that your approach is cleaner.  There appears to be two parts to your work.  
> First, you implemented the determining and passing of the options 
> differently, and secondly, you improved the handling of off-loading and 
> system specific file handling.  Based on your earlier response, we proposed 
> to him the following and he agrees that it seems appropriate.  Could you 
> please add comments to https://reviews.llvm.org/D131469 and he will work with 
> you to change his code to reflect searching for -o and using the virtual 
> functions.  Then, if @MaskRay agrees, he can land his code and finish up his 
> GSoC work.  You can then add your extensions of off-loading and 
> file-handling.  Is this acceptable to you?

Yes, I can gladly do that.

I'm sorry for the confusion this must have caused for you and @dongjunduo, my 
intention was not judgment on the code he posted, rather seeing his code 
motivated me to post my own changes, that I've been working on beforehand., 
Some of his code gave me the right ideas, so I'm happy to share the credit for 
it. Let's make this contribution better together!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133662/new/

https://reviews.llvm.org/D133662

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133662: [Clang] Change -ftime-trace storing path and support multiple compilation jobs

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

I had an email exchange with @dongjunduo about this situation.  He was a GSoC 
student that @Whitney and I were mentoring for the past summer.  He agrees that 
your approach is cleaner.  There appears to be two parts to your work.  First, 
you implemented the determining and passing of the options differently, and 
secondly, you improved the handling of off-loading and system specific file 
handling.  Based on your earlier response, we proposed to him the following and 
he agrees that it seems appropriate.  Could you please add comments to 
https://reviews.llvm.org/D131469 and he will work with you to change his code 
to reflect searching for -o and using the virtual functions.  Then, if @MaskRay 
agrees, he can land his code and finish up his GSoC work.  You can then add 
your extensions of off-loading and file-handling.  Is this acceptable to you?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133662/new/

https://reviews.llvm.org/D133662

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133662: [Clang] Change -ftime-trace storing path and support multiple compilation jobs

[Mészáros Gergely via Phabricator via cfe-commits]

Maetveis updated this revision to Diff 460507.
Maetveis retitled this revision from "[Clang] WIP: Change -ftime-trace storing 
path and support multiple compilation jobs" to "[Clang] Change -ftime-trace 
storing path and support multiple compilation jobs".
Maetveis added a comment.

Remove inline elements from the map. Add tests. With the tests I now consider 
this complete, so removed the draft from the title.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133662/new/

https://reviews.llvm.org/D133662

Files:
  clang/include/clang/Driver/Compilation.h
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/Tool.h
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Clang.h
  clang/test/Driver/check-time-trace.cpp
  clang/tools/driver/cc1_main.cpp

Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -212,9 +212,7 @@
   bool Success = CompilerInvocation::CreateFromArgs(Clang->getInvocation(),
 Argv, Diags, Argv0);
 
-  if (Clang->getFrontendOpts().TimeTrace ||
-  !Clang->getFrontendOpts().TimeTracePath.empty()) {
-Clang->getFrontendOpts().TimeTrace = 1;
+  if (!Clang->getFrontendOpts().TimeTracePath.empty()) {
 llvm::timeTraceProfilerInitialize(
 Clang->getFrontendOpts().TimeTraceGranularity, Argv0);
   }
@@ -256,17 +254,10 @@
   llvm::TimerGroup::clearAll();
 
   if (llvm::timeTraceProfilerEnabled()) {
-SmallString<128> Path(Clang->getFrontendOpts().OutputFile);
-llvm::sys::path::replace_extension(Path, "json");
-if (!Clang->getFrontendOpts().TimeTracePath.empty()) {
-  // replace the suffix to '.json' directly
-  SmallString<128> TracePath(Clang->getFrontendOpts().TimeTracePath);
-  if (llvm::sys::fs::is_directory(TracePath))
-llvm::sys::path::append(TracePath, llvm::sys::path::filename(Path));
-  Path.assign(TracePath);
-}
+const StringRef TracePath = Clang->getFrontendOpts().TimeTracePath;
+assert(!TracePath.empty() && "`-ftime-trace=` is empty");
 if (auto profilerOutput = Clang->createOutputFile(
-Path.str(), /*Binary=*/false, /*RemoveFileOnSignal=*/false,
+TracePath, /*Binary=*/false, /*RemoveFileOnSignal=*/false,
 /*useTemporary=*/false)) {
   llvm::timeTraceProfilerWrite(*profilerOutput);
   profilerOutput.reset();
Index: clang/test/Driver/check-time-trace.cpp
===
--- clang/test/Driver/check-time-trace.cpp
+++ clang/test/Driver/check-time-trace.cpp
@@ -31,6 +31,42 @@
 // CHECK:  "name": "process_name"
 // CHECK:  "name": "thread_name"
 
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: %clangxx -ftime-trace=path/to/trace.json -c -x hip -nogpulib -nogpuinc --offload-arch=gfx803 --offload-arch=gfx900 -### -- %s 2>&1 \
+// RUN:   | FileCheck --check-prefixes=COMMON,FILE %s
+// RUN: %clangxx -ftime-trace=%t -c -x hip -nogpulib -nogpuinc --offload-arch=gfx803 --offload-arch=gfx900 -### -- %s 2>&1 \
+// RUN:   | FileCheck --check-prefixes=COMMON,FOLDER %s
+// RUN: %clangxx -ftime-trace -c -o path/to/output.o -x hip -nogpulib -nogpuinc --offload-arch=gfx803 --offload-arch=gfx900 -### -- %s 2>&1 \
+// RUN:   | FileCheck --check-prefixes=COMMON,OUTPUT %s
+// RUN: %clangxx -ftime-trace -x hip -nogpulib -nogpuinc --offload-arch=gfx803 --offload-arch=gfx900 -### -- %s 2>&1 \
+// RUN:   | FileCheck --check-prefixes=COMMON,NO_OUTPUT %s
+
+// COMMON: "-target-cpu" "gfx803"
+// FILE: "-ftime-trace=path/to/check-time-trace-{{.*}}-gfx803.json"
+// FOLDER: "-ftime-trace=[[BASE:.*]]/check-time-trace-{{.*}}-gfx803.json"
+// OUTPUT: "-ftime-trace=path/to/check-time-trace-{{.*}}-gfx803.json"
+// NO_OUTPUT: "-ftime-trace=check-time-trace-{{.*}}-gfx803.json"
+
+// COMMON: "-target-cpu" "gfx900"
+// FILE: "-ftime-trace=path/to/check-time-trace-{{.*}}-gfx900.json"
+// FOLDER: "-ftime-trace=[[BASE]]/check-time-trace-{{.*}}-gfx900.json"
+// OUTPUT: "-ftime-trace=path/to/check-time-trace-{{.*}}-gfx900.json"
+// NO_OUTPUT: "-ftime-trace=check-time-trace-{{.*}}-gfx900.json"
+
+// COMMON: "-cc1"
+// COMMON-NOT: "-target-cpu" "gfx
+// FILE: "-ftime-trace=path/to/trace.json"
+// FOLDER: "-ftime-trace=[[BASE]]/check-time-trace.json"
+// OUTPUT: "-ftime-trace=path/to/output.json"
+// NO_OUTPUT: "-ftime-trace=check-time-trace-host-{{.*}}.json"
+
+// RUN %clangxx -ftime-trace -save-temps -S -### -- %s 2>&1 | FileCheck --check-prefix=SAVE_TEMPS
+// SAVE_TEMPS: "-E"
+// SAVE_TEMPS: "-ftime-trace=check-time-trace.ii.json"
+// SAVE_TEMPS: "-S"
+// SAVE_TEMPS: "-ftime-trace=check-time-trace.s.json"
+
 template 
 struct Struct {
   T Num;
Index: clang/lib/Driver/ToolChains/Clang.h

[PATCH] D133662: [Clang] Change -ftime-trace storing path and support multiple compilation jobs

[Mészáros Gergely via Phabricator via cfe-commits]

Maetveis created this revision.
Maetveis added a reviewer: clang.
Herald added a project: All.
Maetveis requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

This is an alternative approach for [D131469 
](https://reviews.llvm.org/D131469].

The difference in behavior compared to D131469 
 is that instead of looking for linking jobs, 
here if a "main" output is available ('-o ' is given) the directory of 
it is used for '-ftime-trace'.
Another is that time traces in offloading tool-chain (e.g. CUDA or HIP) 
compilation steps are also supported by saving them to the  same directory as 
the host trace, with naming similar to '-save-temps'.
If save temps the file type suffix is also included to keep the trace from e.g 
the front-end compilation overwriting the back-end trace.

The final behavior is this:

- if '-ftime-trace=path/to/trace.json' is given, then use it for the final 
compilation, and use the directory where it is for the rest (offloading or 
'-save-temps')
- if '-ftime-trace=path/to/trace/folder' is given:
  - for the main output use 'path/to/trace/folder/,json' if '-o 
.o' is given
  - if no output path is specified or for the rest of the files use 
'path/to/trace/folder/.json' similar to '-save-temps'
- if '-ftime-trace' is given and there is a main output ('-o output') use the 
directory of it to store the traces
- if '-ftime-trace' but no '-o ' use the current working directory


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133662

Files:
  clang/include/clang/Driver/Compilation.h
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/Tool.h
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Clang.h
  clang/tools/driver/cc1_main.cpp

Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -212,9 +212,7 @@
   bool Success = CompilerInvocation::CreateFromArgs(Clang->getInvocation(),
 Argv, Diags, Argv0);
 
-  if (Clang->getFrontendOpts().TimeTrace ||
-  !Clang->getFrontendOpts().TimeTracePath.empty()) {
-Clang->getFrontendOpts().TimeTrace = 1;
+  if (!Clang->getFrontendOpts().TimeTracePath.empty()) {
 llvm::timeTraceProfilerInitialize(
 Clang->getFrontendOpts().TimeTraceGranularity, Argv0);
   }
@@ -256,17 +254,10 @@
   llvm::TimerGroup::clearAll();
 
   if (llvm::timeTraceProfilerEnabled()) {
-SmallString<128> Path(Clang->getFrontendOpts().OutputFile);
-llvm::sys::path::replace_extension(Path, "json");
-if (!Clang->getFrontendOpts().TimeTracePath.empty()) {
-  // replace the suffix to '.json' directly
-  SmallString<128> TracePath(Clang->getFrontendOpts().TimeTracePath);
-  if (llvm::sys::fs::is_directory(TracePath))
-llvm::sys::path::append(TracePath, llvm::sys::path::filename(Path));
-  Path.assign(TracePath);
-}
+const StringRef TracePath = Clang->getFrontendOpts().TimeTracePath;
+assert(!TracePath.empty() && "`-ftime-trace=` is empty");
 if (auto profilerOutput = Clang->createOutputFile(
-Path.str(), /*Binary=*/false, /*RemoveFileOnSignal=*/false,
+TracePath, /*Binary=*/false, /*RemoveFileOnSignal=*/false,
 /*useTemporary=*/false)) {
   llvm::timeTraceProfilerWrite(*profilerOutput);
   profilerOutput.reset();
Index: clang/lib/Driver/ToolChains/Clang.h
===
--- clang/lib/Driver/ToolChains/Clang.h
+++ clang/lib/Driver/ToolChains/Clang.h
@@ -113,6 +113,7 @@
   bool hasIntegratedBackend() const override { return HasBackend; }
   bool hasIntegratedCPP() const override { return true; }
   bool canEmitIR() const override { return true; }
+  bool supportsTimeTrace() const override { return true; }
 
   void ConstructJob(Compilation , const JobAction ,
 const InputInfo , const InputInfoList ,
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -6230,9 +6230,11 @@
   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_EQ);
+
+  if (const char *Name = C.getTimeTraceFileForJob())
+CmdArgs.push_back(Args.MakeArgString("-ftime-trace=" + Twine(Name)));
+
   Args.AddLastArg(CmdArgs,