[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D89087#2359962 <https://reviews.llvm.org/D89087#2359962>, @davidxl wrote:

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

In D89087#2359977 <https://reviews.llvm.org/D89087#2359977>, @davidxl wrote:

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



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:587
 
-  if (CodeGenOpts.CFProtectionBranch &&
-      Target.checkCFProtectionBranchSupported(getDiags())) {
-    // Indicate that we want to instrument branch control flow protection.
-    getModule().addModuleFlag(llvm::Module::Override, "cf-protection-branch",
+  if (CodeGenOpts.CFProtectionReturn &&
+      Target.checkCFProtectionReturnSupported(getDiags())) {
----------------
davidxl wrote:
> Is this change relevant?
Woops, looks like a bad merge! Will fix.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:613
+    llvm::LLVMContext &Ctx = TheModule.getContext();
+    getModule().addModuleFlag(
+        llvm::Module::Error, "MemProfProfileFilename",
----------------
davidxl wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89087