Page MenuHomePhabricator

[Clang] Change -ftime-trace storing path and support multiple compilation jobs
Needs ReviewPublic

Authored by Maetveis on Sep 11 2022, 2:34 AM.

Details

Reviewers
jamieschmeiser
Whitney
steven_wu
MaskRay
dongjunduo
Group Reviewers
Restricted Project
Summary

This is an alternative approach for D131469.

The difference in behavior compared to D131469 is that instead of looking for linking jobs, here if a "main" output is available ('-o <output>' 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' is on 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/<output-name>,json' if '-o <output-name>.o' is given
    • if no output path is specified or for the rest of the files use 'path/to/trace/folder/<input-name>.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 <output>' use the current working directory

The patch also drops '-ftime-trace' from cc1 options as the driver now always passes the full path in '-ftime-trace=<path>'

Diff Detail

Event Timeline

Maetveis created this revision.Sep 11 2022, 2:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2022, 2:34 AM
Maetveis requested review of this revision.Sep 11 2022, 2:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2022, 2:34 AM
Maetveis edited the summary of this revision. (Show Details)Sep 11 2022, 2:34 AM
Maetveis edited the summary of this revision. (Show Details)
Maetveis edited the summary of this revision. (Show Details)
Maetveis edited the summary of this revision. (Show Details)
Maetveis edited the summary of this revision. (Show Details)Sep 11 2022, 2:41 AM
Maetveis retitled this revision from [Clang] Change -ftime-trace storing path and support multiple compilation jobs to [Clang] WIP: Change -ftime-trace storing path and support multiple compilation jobs.Sep 11 2022, 6:58 AM
dongjunduo added a subscriber: dongjunduo.
dongjunduo removed a subscriber: dongjunduo.

I'm a little confused as to what is being proposed here. Is this building on D131469 or is it an alternative? It seems that there are portions of the code from D131469 included in these changes, which implies that you are building on it. I think this patch is premature in that the other patch has not yet landed (@MaskRay has asked for revisions that @dongjunduo has made but is waiting for review). When that patch has landed, this could be reposted based on those changes.

I'm a little confused as to what is being proposed here. Is this building on D131469 or is it an alternative? It seems that there are portions of the code from D131469 included in these changes, which implies that you are building on it. I think this patch is premature in that the other patch has not yet landed (@MaskRay has asked for revisions that @dongjunduo has made but is waiting for review). When that patch has landed, this could be reposted based on those changes.

Yes, there is some code taken from D131469, mainly removing the -ftime-trace file logic from cc1.
The way the paths on where to store the traces are different both in implementation and in behaviour.
D131469 is rewriting the arguements for already created jobs and infers the path based on a link job.
This patch sets up the time trace path for each job that supports it, so it can be read during assembling of the commands. The location is based on the final output (-o <out>) and the file names are based on the inputs instead of the output names (similar to -save-temps).

I felt that this is a different approach, that might not be taken once D131469 is accepted. If it's better to leave these points as suggestions on D131469, than I can summarize them over there.
Waiting for it to land is also fine for me.

At a glance the approach looks more elegant to me, but I haven't studied this carefully yet. Also thanks for checking how to handle offloading.

MaskRay added inline comments.Sep 13 2022, 10:27 PM
clang/include/clang/Driver/Compilation.h
134

I don't think we should use a non-zero inline element for these members.
They are, in 99.9% cases, empty. We should optimize for the member size by making the number of inline element to 0.

Maetveis updated this revision to Diff 460507.Sep 15 2022, 2:07 PM
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.

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

Maetveis marked an inline comment as done.Sep 15 2022, 2:07 PM

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?

Maetveis added a comment.EditedSep 18 2022, 11:21 AM

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 his contribution better together!

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 &T, Args &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.