[PATCH] D151593: [MemProf] Clean up MemProf instrumentation pass invocation

[Teresa Johnson via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf354e971b09c: [MemProf] Clean up MemProf instrumentation 
pass invocation (authored by tejohnson).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151593

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/lib/Passes/PassBuilderPipelines.cpp


Index: llvm/lib/Passes/PassBuilderPipelines.cpp
===
--- llvm/lib/Passes/PassBuilderPipelines.cpp
+++ llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -155,9 +155,6 @@
 cl::Hidden,
 cl::desc("Enable inline deferral during PGO"));
 
-static cl::opt EnableMemProfiler("enable-mem-prof", cl::Hidden,
-   cl::desc("Enable memory profiler"));
-
 static cl::opt EnableModuleInliner("enable-module-inliner",
  cl::init(false), cl::Hidden,
  cl::desc("Enable module inliner"));
@@ -1122,11 +1119,6 @@
   MPM.addPass(GlobalOptPass());
   MPM.addPass(GlobalDCEPass());
 
-  if (EnableMemProfiler && Phase != ThinOrFullLTOPhase::ThinLTOPreLink) {
-MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
-MPM.addPass(ModuleMemProfilerPass());
-  }
-
   return MPM;
 }
 
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -992,6 +992,16 @@
 MPM.addPass(InstrProfiling(*Options, false));
   });
 
+// TODO: Consider passing the MemoryProfileOutput to the pass builder via
+// the PGOOptions, and set this up there.
+if (!CodeGenOpts.MemoryProfileOutput.empty()) {
+  PB.registerOptimizerLastEPCallback(
+  [](ModulePassManager , OptimizationLevel Level) {
+MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
+MPM.addPass(ModuleMemProfilerPass());
+  });
+}
+
 if (IsThinLTO) {
   MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
 } else if (IsLTO) {
@@ -999,11 +1009,6 @@
 } else {
   MPM = PB.buildPerModuleDefaultPipeline(Level);
 }
-
-if (!CodeGenOpts.MemoryProfileOutput.empty()) {
-  MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
-  MPM.addPass(ModuleMemProfilerPass());
-}
   }
 
   // Add a verifier pass if requested. We don't have to do this if the action


Index: llvm/lib/Passes/PassBuilderPipelines.cpp
===
--- llvm/lib/Passes/PassBuilderPipelines.cpp
+++ llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -155,9 +155,6 @@
 cl::Hidden,
 cl::desc("Enable inline deferral during PGO"));
 
-static cl::opt EnableMemProfiler("enable-mem-prof", cl::Hidden,
-   cl::desc("Enable memory profiler"));
-
 static cl::opt EnableModuleInliner("enable-module-inliner",
  cl::init(false), cl::Hidden,
  cl::desc("Enable module inliner"));
@@ -1122,11 +1119,6 @@
   MPM.addPass(GlobalOptPass());
   MPM.addPass(GlobalDCEPass());
 
-  if (EnableMemProfiler && Phase != ThinOrFullLTOPhase::ThinLTOPreLink) {
-MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
-MPM.addPass(ModuleMemProfilerPass());
-  }
-
   return MPM;
 }
 
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -992,6 +992,16 @@
 MPM.addPass(InstrProfiling(*Options, false));
   });
 
+// TODO: Consider passing the MemoryProfileOutput to the pass builder via
+// the PGOOptions, and set this up there.
+if (!CodeGenOpts.MemoryProfileOutput.empty()) {
+  PB.registerOptimizerLastEPCallback(
+  [](ModulePassManager , OptimizationLevel Level) {
+MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
+MPM.addPass(ModuleMemProfilerPass());
+  });
+}
+
 if (IsThinLTO) {
   MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
 } else if (IsLTO) {
@@ -999,11 +1009,6 @@
 } else {
   MPM = PB.buildPerModuleDefaultPipeline(Level);
 }
-
-if (!CodeGenOpts.MemoryProfileOutput.empty()) {
-  MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
-  MPM.addPass(ModuleMemProfilerPass());
-}
   }
 
   // Add a verifier pass if requested. We don't have to do this if the action
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D151593: [MemProf] Clean up MemProf instrumentation pass invocation

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish accepted this revision.
snehasish added a comment.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151593

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

[PATCH] D151593: [MemProf] Clean up MemProf instrumentation pass invocation

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:995
 
+// TODO: Consider passing the MemoryProfileOutput to the pass builder via
+// the PGOOptions, and set this up there.

vitalybuka wrote:
> tejohnson wrote:
> > vitalybuka wrote:
> > > if this is registerOptimizerLastEPCallback, it can be next to other 
> > > sanitizers, in addSanitizers()
> > It could be, but while it uses the sanitizer common support, it isn't 
> > logically a sanitizer. Is there an advantage to setting it up there?
> no advantages.
> it has runtime similar to sanitizer, so I rather thing about this as 
> sanitizer :)
> 
Ok thanks. I think I would prefer to keep it separate, and hopefully soon 
address the TODO here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151593

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

[PATCH] D151593: [MemProf] Clean up MemProf instrumentation pass invocation

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka accepted this revision.
vitalybuka added a comment.
This revision is now accepted and ready to land.

Either way is fine


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151593

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

[PATCH] D151593: [MemProf] Clean up MemProf instrumentation pass invocation

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:995
 
+// TODO: Consider passing the MemoryProfileOutput to the pass builder via
+// the PGOOptions, and set this up there.

tejohnson wrote:
> vitalybuka wrote:
> > if this is registerOptimizerLastEPCallback, it can be next to other 
> > sanitizers, in addSanitizers()
> It could be, but while it uses the sanitizer common support, it isn't 
> logically a sanitizer. Is there an advantage to setting it up there?
no advantages.
it has runtime similar to sanitizer, so I rather thing about this as sanitizer 
:)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151593

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

[PATCH] D151593: [MemProf] Clean up MemProf instrumentation pass invocation

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:995
 
+// TODO: Consider passing the MemoryProfileOutput to the pass builder via
+// the PGOOptions, and set this up there.

vitalybuka wrote:
> if this is registerOptimizerLastEPCallback, it can be next to other 
> sanitizers, in addSanitizers()
It could be, but while it uses the sanitizer common support, it isn't logically 
a sanitizer. Is there an advantage to setting it up there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151593

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

[PATCH] D151593: [MemProf] Clean up MemProf instrumentation pass invocation

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:995
 
+// TODO: Consider passing the MemoryProfileOutput to the pass builder via
+// the PGOOptions, and set this up there.

if this is registerOptimizerLastEPCallback, it can be next to other sanitizers, 
in addSanitizers()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151593

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

[PATCH] D151593: [MemProf] Clean up MemProf instrumentation pass invocation

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson created this revision.
tejohnson added a reviewer: snehasish.
Herald added subscribers: ormris, hiraditya.
Herald added a project: All.
tejohnson requested review of this revision.
Herald added projects: clang, LLVM.

First, removes the invocation of the memprof instrumentation passes from
the end of the module simplification pass builder, where it doesn't
really belong. However, it turns out that this was never being invoked,
as it is guarded by an internal option not used anywhere (even tests).

These passes are actually added via clang under the -fmemory-profile
option. Changed this to add via the EP callback interface, similar to
the sanitizer passes. They are added to the EP for the end of the
optimization pipeline, which is roughly where they were being added
already (end of the pre-LTO link pipelines and non-LTO optimization
pipeline).

Ideally we should plumb the output file through to LLVM and set it up
there, so I have added a TODO.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151593

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/lib/Passes/PassBuilderPipelines.cpp


Index: llvm/lib/Passes/PassBuilderPipelines.cpp
===
--- llvm/lib/Passes/PassBuilderPipelines.cpp
+++ llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -155,9 +155,6 @@
 cl::Hidden,
 cl::desc("Enable inline deferral during PGO"));
 
-static cl::opt EnableMemProfiler("enable-mem-prof", cl::Hidden,
-   cl::desc("Enable memory profiler"));
-
 static cl::opt EnableModuleInliner("enable-module-inliner",
  cl::init(false), cl::Hidden,
  cl::desc("Enable module inliner"));
@@ -1122,11 +1119,6 @@
   MPM.addPass(GlobalOptPass());
   MPM.addPass(GlobalDCEPass());
 
-  if (EnableMemProfiler && Phase != ThinOrFullLTOPhase::ThinLTOPreLink) {
-MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
-MPM.addPass(ModuleMemProfilerPass());
-  }
-
   return MPM;
 }
 
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -992,6 +992,16 @@
 MPM.addPass(InstrProfiling(*Options, false));
   });
 
+// TODO: Consider passing the MemoryProfileOutput to the pass builder via
+// the PGOOptions, and set this up there.
+if (!CodeGenOpts.MemoryProfileOutput.empty()) {
+  PB.registerOptimizerLastEPCallback(
+  [](ModulePassManager , OptimizationLevel Level) {
+MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
+MPM.addPass(ModuleMemProfilerPass());
+  });
+}
+
 if (IsThinLTO) {
   MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
 } else if (IsLTO) {
@@ -999,11 +1009,6 @@
 } else {
   MPM = PB.buildPerModuleDefaultPipeline(Level);
 }
-
-if (!CodeGenOpts.MemoryProfileOutput.empty()) {
-  MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
-  MPM.addPass(ModuleMemProfilerPass());
-}
   }
 
   // Add a verifier pass if requested. We don't have to do this if the action


Index: llvm/lib/Passes/PassBuilderPipelines.cpp
===
--- llvm/lib/Passes/PassBuilderPipelines.cpp
+++ llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -155,9 +155,6 @@
 cl::Hidden,
 cl::desc("Enable inline deferral during PGO"));
 
-static cl::opt EnableMemProfiler("enable-mem-prof", cl::Hidden,
-   cl::desc("Enable memory profiler"));
-
 static cl::opt EnableModuleInliner("enable-module-inliner",
  cl::init(false), cl::Hidden,
  cl::desc("Enable module inliner"));
@@ -1122,11 +1119,6 @@
   MPM.addPass(GlobalOptPass());
   MPM.addPass(GlobalDCEPass());
 
-  if (EnableMemProfiler && Phase != ThinOrFullLTOPhase::ThinLTOPreLink) {
-MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
-MPM.addPass(ModuleMemProfilerPass());
-  }
-
   return MPM;
 }
 
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -992,6 +992,16 @@
 MPM.addPass(InstrProfiling(*Options, false));
   });
 
+// TODO: Consider passing the MemoryProfileOutput to the pass builder via
+// the PGOOptions, and set this up there.
+if (!CodeGenOpts.MemoryProfileOutput.empty()) {
+  PB.registerOptimizerLastEPCallback(
+  [](ModulePassManager , OptimizationLevel Level) {
+MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
+