Page MenuHomePhabricator

[MemProf] Pass down memory profile name with optional path from clang
ClosedPublic

Authored by tejohnson on Oct 8 2020, 5:56 PM.

Details

Summary

Similar to -fprofile-generate=, add -fmemory-profile= which takes a
directory path. This is passed down to LLVM via a new module flag
metadata. LLVM in turn provides this name to the runtime via the new
__memprof_profile_filename variable.

Additionally, always pass a default filename (in $cwd if a directory
name is not specified vi the = form of the option). This is also
consistent with the behavior of the PGO instrumentation. Since the
memory profiles will generally be fairly large, it doesn't make sense to
dump them to stderr. Also, importantly, the memory profiles will
eventually be dumped in a compact binary format, which is another reason
why it does not make sense to send these to stderr by default.

Depends on D89086.

Diff Detail

Event Timeline

tejohnson created this revision.Oct 8 2020, 5:56 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 8 2020, 5:56 PM
tejohnson requested review of this revision.Oct 8 2020, 5:56 PM

As discussed in review for D89086, move test changes to this patch since they are only required here.

Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2020, 12:50 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

There should be a related LLVM side of changes. Is it in a different patch?

clang/lib/CodeGen/CodeGenModule.cpp
589

Is this change relevant?

612

Is there a need for a module flag? PGO passes an InstrProfOptions typed object to the instrumentation pass. The option object has the directory info.

longer term, the profile will be dumped into PGO's raw file, so for now is there a need for a user level option? should an internal option good enough?

There should be a related LLVM side of changes. Is it in a different patch?

That's here too. See the change in llvm/lib/Transforms/Instrumentation/MemProfiler.cpp and a related test (they are near the bottom of the patch below all the clang tests).

longer term, the profile will be dumped into PGO's raw file, so for now is there a need for a user level option? should an internal option good enough?

I think it would be good to keep an option to specify output for this profile independently. Both for users that want to only collect a memory profile in a single build/run, and for users that may want to keep it separate for some reason (and us for now).

clang/lib/CodeGen/CodeGenModule.cpp
589

Woops, looks like a bad merge! Will fix.

612

I was trying to avoid going the route PGO uses to pass that down, as it is different between the old and new PM and I think more complex than necessary as a result.

tejohnson updated this revision to Diff 302112.Sat, Oct 31, 2:12 PM

Rebase and fix bad merge

davidxl accepted this revision.Sat, Oct 31, 4:32 PM

lgtm

This revision is now accepted and ready to land.Sat, Oct 31, 4:32 PM
MaskRay added inline comments.
clang/include/clang/Driver/Options.td
1021

DriverOption has been renamed to NoXarchOption (which is just used to provide a diagnostic for -Xarch, which may be removed in the future). Please remove the flag

clang/lib/Driver/ToolChains/Clang.cpp
4316

if not match options::OPT_fno_memory_profile, render(Args, CmdArgs)

clang/lib/Frontend/CompilerInvocation.cpp
1042

const StringLiteral

1049

std::string(MemProfileBasename)

tejohnson marked 3 inline comments as done.Sat, Oct 31, 10:47 PM
tejohnson added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1049

This change doesn't seem to be needed?

Address comments

MaskRay accepted this revision.Sat, Oct 31, 11:55 PM
This revision was landed with ongoing or failed builds.Sun, Nov 1, 5:39 PM
This revision was automatically updated to reflect the committed changes.