Manually create unique_ptr in various pass adaptors

[aeubanks]

Summary

This avoids creating tons of make_unique template instantiations. And we
only create a unique_ptr of the actual pass concept type, rather than
creating a unique_ptr of the pass model subclass then casting it to the
pass concept type.

This reduces the work spent compiling PassBuilder.cpp from 83M -> 73M
instructions according to perf stat.

[rnk]

lgtm

This seems relevant:
https://twitter.com/reidkleckner/status/1198108401177251840

[dblaikie]

"83M -> 73M" is peak RAM usage during compilation, I guess?

Is that worth the code changes? I think there's value in make_unique making it easier to read code/check for memory leaks, etc. (makes explicit new stand out more/get extra scrutiny when reading code)

[aeubanks]

In D110784#3040708, @dblaikie wrote:

"83M -> 73M" is peak RAM usage during compilation, I guess?

Is that worth the code changes? I think there's value in make_unique making it easier to read code/check for memory leaks, etc. (makes explicit new stand out more/get extra scrutiny when reading code)

That's compile time, specifically the "instructions" stat in perf stat (I should have specified more clearly in the description).
PassBuilder.cpp is by far the longest file to compile (if you only build X86). With many cores/distributed build farms, it's the bottleneck. So yeah I think it's worth it.

[dblaikie]

In D110784#3040749, @aeubanks wrote:

In D110784#3040708, @dblaikie wrote:

"83M -> 73M" is peak RAM usage during compilation, I guess?

Is that worth the code changes? I think there's value in make_unique making it easier to read code/check for memory leaks, etc. (makes explicit new stand out more/get extra scrutiny when reading code)

That's compile time, specifically the "instructions" stat in perf stat (I should have specified more clearly in the description).
PassBuilder.cpp is by far the longest file to compile (if you only build X86). With many cores/distributed build farms, it's the bottleneck. So yeah I think it's worth it.

Ah, sorry, didn't catch the "instructions" on the line wrap there. I see.

What about splitting up the file in some way(s)?

[aeubanks]

In D110784#3040750, @dblaikie wrote:

In D110784#3040749, @aeubanks wrote:

In D110784#3040708, @dblaikie wrote:

"83M -> 73M" is peak RAM usage during compilation, I guess?

Is that worth the code changes? I think there's value in make_unique making it easier to read code/check for memory leaks, etc. (makes explicit new stand out more/get extra scrutiny when reading code)

That's compile time, specifically the "instructions" stat in perf stat (I should have specified more clearly in the description).
PassBuilder.cpp is by far the longest file to compile (if you only build X86). With many cores/distributed build farms, it's the bottleneck. So yeah I think it's worth it.

Ah, sorry, didn't catch the "instructions" on the line wrap there. I see.

What about splitting up the file in some way(s)?

We can't get around (at least with the current design) tons of instantiations of these because for each entry in PassRegistry.def we instantiate at least one of these adaptors, sometimes multiple ones. Splitting these up into multiple files doesn't make sense logically from a code organization point of view, since they're all about parsing a pass pipeline and a lot of the code is very similar between module/cgscc/function/loop passes.

I have tried splitting things out before with no real impact, although I can look again.

[dblaikie]

In D110784#3040848, @aeubanks wrote:

In D110784#3040750, @dblaikie wrote:

In D110784#3040749, @aeubanks wrote:

In D110784#3040708, @dblaikie wrote:

"83M -> 73M" is peak RAM usage during compilation, I guess?

Is that worth the code changes? I think there's value in make_unique making it easier to read code/check for memory leaks, etc. (makes explicit new stand out more/get extra scrutiny when reading code)

That's compile time, specifically the "instructions" stat in perf stat (I should have specified more clearly in the description).
PassBuilder.cpp is by far the longest file to compile (if you only build X86). With many cores/distributed build farms, it's the bottleneck. So yeah I think it's worth it.

Ah, sorry, didn't catch the "instructions" on the line wrap there. I see.

What about splitting up the file in some way(s)?

We can't get around (at least with the current design) tons of instantiations of these because for each entry in PassRegistry.def we instantiate at least one of these adaptors, sometimes multiple ones. Splitting these up into multiple files doesn't make sense logically from a code organization point of view, since they're all about parsing a pass pipeline and a lot of the code is very similar between module/cgscc/function/loop passes.

Yeah, I don't think it'd avoid any instantiations, but could improve build parallelism.

If these explicit new+unique_ptr's are going to stay, they might warrant comments so someone doesn't come along and clean them up again? (I think we have clang-tidy checks that would probably identify this code and undo the work of this commit, for instance - and sometimes I go looking for uses of explicit new to see if they can be moved to more automatic memory management).

I have tried splitting things out before with no real impact, although I can look again.

No real impact on the compile time of each file separately/added together? (I think splitting a single long-compile, even if it adds some total time (eg: the compile time of the split files adds up to more than the compile time of the single file) can still be worthwhile for build parallelism)

[aeubanks]

In D110784#3040968, @dblaikie wrote:

In D110784#3040848, @aeubanks wrote:

In D110784#3040750, @dblaikie wrote:

In D110784#3040749, @aeubanks wrote:

In D110784#3040708, @dblaikie wrote:

"83M -> 73M" is peak RAM usage during compilation, I guess?

Is that worth the code changes? I think there's value in make_unique making it easier to read code/check for memory leaks, etc. (makes explicit new stand out more/get extra scrutiny when reading code)

That's compile time, specifically the "instructions" stat in perf stat (I should have specified more clearly in the description).
PassBuilder.cpp is by far the longest file to compile (if you only build X86). With many cores/distributed build farms, it's the bottleneck. So yeah I think it's worth it.

Ah, sorry, didn't catch the "instructions" on the line wrap there. I see.

What about splitting up the file in some way(s)?

We can't get around (at least with the current design) tons of instantiations of these because for each entry in PassRegistry.def we instantiate at least one of these adaptors, sometimes multiple ones. Splitting these up into multiple files doesn't make sense logically from a code organization point of view, since they're all about parsing a pass pipeline and a lot of the code is very similar between module/cgscc/function/loop passes.

Yeah, I don't think it'd avoid any instantiations, but could improve build parallelism.

If these explicit new+unique_ptr's are going to stay, they might warrant comments so someone doesn't come along and clean them up again? (I think we have clang-tidy checks that would probably identify this code and undo the work of this commit, for instance - and sometimes I go looking for uses of explicit new to see if they can be moved to more automatic memory management).

Yeah they already have comments.

I have tried splitting things out before with no real impact, although I can look again.

No real impact on the compile time of each file separately/added together? (I think splitting a single long-compile, even if it adds some total time (eg: the compile time of the split files adds up to more than the compile time of the single file) can still be worthwhile for build parallelism)

I split out some of the largest functions into PassBuilderPipelines.cpp and there was no measurable change in the compile time of PassBuilder.cpp. (I still submitted that because it was nice to separate it out)

[dblaikie]

In D110784#3041091, @aeubanks wrote:

In D110784#3040968, @dblaikie wrote:

In D110784#3040848, @aeubanks wrote:

In D110784#3040750, @dblaikie wrote:

In D110784#3040749, @aeubanks wrote:

In D110784#3040708, @dblaikie wrote:

"83M -> 73M" is peak RAM usage during compilation, I guess?

Is that worth the code changes? I think there's value in make_unique making it easier to read code/check for memory leaks, etc. (makes explicit new stand out more/get extra scrutiny when reading code)

That's compile time, specifically the "instructions" stat in perf stat (I should have specified more clearly in the description).
PassBuilder.cpp is by far the longest file to compile (if you only build X86). With many cores/distributed build farms, it's the bottleneck. So yeah I think it's worth it.

Ah, sorry, didn't catch the "instructions" on the line wrap there. I see.

What about splitting up the file in some way(s)?

We can't get around (at least with the current design) tons of instantiations of these because for each entry in PassRegistry.def we instantiate at least one of these adaptors, sometimes multiple ones. Splitting these up into multiple files doesn't make sense logically from a code organization point of view, since they're all about parsing a pass pipeline and a lot of the code is very similar between module/cgscc/function/loop passes.

Yeah, I don't think it'd avoid any instantiations, but could improve build parallelism.

If these explicit new+unique_ptr's are going to stay, they might warrant comments so someone doesn't come along and clean them up again? (I think we have clang-tidy checks that would probably identify this code and undo the work of this commit, for instance - and sometimes I go looking for uses of explicit new to see if they can be moved to more automatic memory management).

Yeah they already have comments.

Ah, sorry, looked at the last ones - seems the comment was missed in the last couple of ones in LoopPassManager.h

I have tried splitting things out before with no real impact, although I can look again.

No real impact on the compile time of each file separately/added together? (I think splitting a single long-compile, even if it adds some total time (eg: the compile time of the split files adds up to more than the compile time of the single file) can still be worthwhile for build parallelism)

I split out some of the largest functions into PassBuilderPipelines.cpp and there was no measurable change in the compile time of PassBuilder.cpp. (I still submitted that because it was nice to separate it out)

Ah, fair enough :/ happy to throw around ideas. It might be that it'd be more improvable by splitting into chunks of passes, rather than splitting functions (eg: rather than one file with the X operation for all passes and one file with the Y operation for all passes - instead one file with X and Y operations for the first half of passes, and another for the second - not that simple, I get, but a thought)

[aeubanks]

In D110784#3041106, @dblaikie wrote:

In D110784#3041091, @aeubanks wrote:

In D110784#3040968, @dblaikie wrote:

In D110784#3040848, @aeubanks wrote:

In D110784#3040750, @dblaikie wrote:

In D110784#3040749, @aeubanks wrote:

In D110784#3040708, @dblaikie wrote:

"83M -> 73M" is peak RAM usage during compilation, I guess?

Is that worth the code changes? I think there's value in make_unique making it easier to read code/check for memory leaks, etc. (makes explicit new stand out more/get extra scrutiny when reading code)

That's compile time, specifically the "instructions" stat in perf stat (I should have specified more clearly in the description).
PassBuilder.cpp is by far the longest file to compile (if you only build X86). With many cores/distributed build farms, it's the bottleneck. So yeah I think it's worth it.

Ah, sorry, didn't catch the "instructions" on the line wrap there. I see.

What about splitting up the file in some way(s)?

We can't get around (at least with the current design) tons of instantiations of these because for each entry in PassRegistry.def we instantiate at least one of these adaptors, sometimes multiple ones. Splitting these up into multiple files doesn't make sense logically from a code organization point of view, since they're all about parsing a pass pipeline and a lot of the code is very similar between module/cgscc/function/loop passes.

Yeah, I don't think it'd avoid any instantiations, but could improve build parallelism.

If these explicit new+unique_ptr's are going to stay, they might warrant comments so someone doesn't come along and clean them up again? (I think we have clang-tidy checks that would probably identify this code and undo the work of this commit, for instance - and sometimes I go looking for uses of explicit new to see if they can be moved to more automatic memory management).

Yeah they already have comments.

Ah, sorry, looked at the last ones - seems the comment was missed in the last couple of ones in LoopPassManager.h

I added the comment to the remaining places.

I have tried splitting things out before with no real impact, although I can look again.

No real impact on the compile time of each file separately/added together? (I think splitting a single long-compile, even if it adds some total time (eg: the compile time of the split files adds up to more than the compile time of the single file) can still be worthwhile for build parallelism)

I split out some of the largest functions into PassBuilderPipelines.cpp and there was no measurable change in the compile time of PassBuilder.cpp. (I still submitted that because it was nice to separate it out)

Ah, fair enough :/ happy to throw around ideas. It might be that it'd be more improvable by splitting into chunks of passes, rather than splitting functions (eg: rather than one file with the X operation for all passes and one file with the Y operation for all passes - instead one file with X and Y operations for the first half of passes, and another for the second - not that simple, I get, but a thought)

Some numbers since I was curious:
Removing everything but the includes: 15B instructions.
Removing parseModule/CGSCC/Function/LoopPass: 37B instructions.
Current state: 80B instructions.

I don't think it makes sense to split the parse*Pass() methods, they have a lot of similar code and it'd be unintuitive to have to update them if they were separate. And almost all the remaining code is to support those methods.
And that's how you'd probably split the passes, by splitting them per IR unit type. I don't think any other way would make sense.

llvm/include/llvm/Analysis/CGSCCPassManager.h

 /// templated adaptor.
 template <typename CGSCCPassT>
 ModuleToPostOrderCGSCCPassAdaptor
 createModuleToPostOrderCGSCCPassAdaptor(CGSCCPassT &&Pass) {
   using PassModelT = detail::PassModel<LazyCallGraph::SCC, CGSCCPassT,
                                        PreservedAnalyses, CGSCCAnalysisManager,
                                        LazyCallGraph &, CGSCCUpdateResult &>;
   return ModuleToPostOrderCGSCCPassAdaptor(
-      std::make_unique<PassModelT>(std::forward<CGSCCPassT>(Pass)));
+      std::unique_ptr<ModuleToPostOrderCGSCCPassAdaptor::PassConceptT>(
+          new PassModelT(std::forward<CGSCCPassT>(Pass))));
 }
 
 /// A proxy from a \c FunctionAnalysisManager to an \c SCC.
 ///
 /// When a module pass runs and triggers invalidation, both the CGSCC and
 /// Function analysis manager proxies on the module get an invalidation event.
 /// We don't want to fully duplicate responsibility for most of the
 /// invalidation logic. Instead, this layer is only responsible for SCC-local

 /// templated adaptor.
 template <typename FunctionPassT>
 CGSCCToFunctionPassAdaptor
 createCGSCCToFunctionPassAdaptor(FunctionPassT &&Pass) {
   using PassModelT =
       detail::PassModel<Function, FunctionPassT, PreservedAnalyses,
                         FunctionAnalysisManager>;
   return CGSCCToFunctionPassAdaptor(
-      std::make_unique<PassModelT>(std::forward<FunctionPassT>(Pass)));
+      std::unique_ptr<CGSCCToFunctionPassAdaptor::PassConceptT>(
+          new PassModelT(std::forward<FunctionPassT>(Pass))));
 }
 
 /// A helper that repeats an SCC pass each time an indirect call is refined to
 /// a direct call by that pass.
 ///
 /// While the CGSCC pass manager works to re-visit SCCs and RefSCCs as they
 /// change shape, we may also want to repeat an SCC pass if it simply refines
 /// an indirect call to a direct call, even if doing so does not alter the

 /// templated adaptor.
 template <typename CGSCCPassT>
 DevirtSCCRepeatedPass createDevirtSCCRepeatedPass(CGSCCPassT &&Pass,
                                                   int MaxIterations) {
   using PassModelT = detail::PassModel<LazyCallGraph::SCC, CGSCCPassT,
                                        PreservedAnalyses, CGSCCAnalysisManager,
                                        LazyCallGraph &, CGSCCUpdateResult &>;
   return DevirtSCCRepeatedPass(
-      std::make_unique<PassModelT>(std::forward<CGSCCPassT>(Pass)),
+      std::unique_ptr<DevirtSCCRepeatedPass::PassConceptT>(
+          new PassModelT(std::forward<CGSCCPassT>(Pass))),
       MaxIterations);
 }
 
 // Clear out the debug logging macro.
 #undef DEBUG_TYPE
 
 } // end namespace llvm
 
 #endif // LLVM_ANALYSIS_CGSCCPASSMANAGER_H

llvm/include/llvm/IR/PassManager.h

 template <typename FunctionPassT>
 ModuleToFunctionPassAdaptor
 createModuleToFunctionPassAdaptor(FunctionPassT &&Pass) {
   using PassModelT =
       detail::PassModel<Function, FunctionPassT, PreservedAnalyses,
                         FunctionAnalysisManager>;
 
   return ModuleToFunctionPassAdaptor(
-      std::make_unique<PassModelT>(std::forward<FunctionPassT>(Pass)));
+      std::unique_ptr<ModuleToFunctionPassAdaptor::PassConceptT>(
+          new PassModelT(std::forward<FunctionPassT>(Pass))));
 }
 
 /// A utility pass template to force an analysis result to be available.
 ///
 /// If there are extra arguments at the pass's run level there may also be
 /// extra arguments to the analysis manager's \c getResult routine. We can't
 /// guess how to effectively map the arguments from one to the other, and so
 /// this specialization just ignores them.

llvm/include/llvm/Transforms/Scalar/LoopPassManager.h

 inline std::enable_if_t<is_detected<HasRunOnLoopT, LoopPassT>::value,
                         FunctionToLoopPassAdaptor>
 createFunctionToLoopPassAdaptor(LoopPassT &&Pass, bool UseMemorySSA = false,
                                 bool UseBlockFrequencyInfo = false) {
   using PassModelT =
       detail::PassModel<Loop, LoopPassT, PreservedAnalyses, LoopAnalysisManager,
                         LoopStandardAnalysisResults &, LPMUpdater &>;
   return FunctionToLoopPassAdaptor(
-      std::make_unique<PassModelT>(std::forward<LoopPassT>(Pass)), UseMemorySSA,
-      UseBlockFrequencyInfo, false);
+      std::unique_ptr<FunctionToLoopPassAdaptor::PassConceptT>(
+          new PassModelT(std::forward<LoopPassT>(Pass))),
+      UseMemorySSA, UseBlockFrequencyInfo, false);
 }
 
 /// If \p Pass is a loop-nest pass, \p Pass will first be wrapped into a
 /// \c LoopPassManager and the returned adaptor will be in loop-nest mode.
 template <typename LoopNestPassT>
 inline std::enable_if_t<!is_detected<HasRunOnLoopT, LoopNestPassT>::value,
                         FunctionToLoopPassAdaptor>
 createFunctionToLoopPassAdaptor(LoopNestPassT &&Pass, bool UseMemorySSA = false,
                                 bool UseBlockFrequencyInfo = false) {
   LoopPassManager LPM;
   LPM.addPass(std::forward<LoopNestPassT>(Pass));
   using PassModelT =
       detail::PassModel<Loop, LoopPassManager, PreservedAnalyses,
                         LoopAnalysisManager, LoopStandardAnalysisResults &,
                         LPMUpdater &>;
-  return FunctionToLoopPassAdaptor(std::make_unique<PassModelT>(std::move(LPM)),
-                                   UseMemorySSA, UseBlockFrequencyInfo, true);
+  return FunctionToLoopPassAdaptor(
+      std::unique_ptr<FunctionToLoopPassAdaptor::PassConceptT>(
+          new PassModelT(std::move(LPM))),
 }
 
 /// If \p Pass is an instance of \c LoopPassManager, the returned adaptor will
 /// be in loop-nest mode if the pass manager contains only loop-nest passes.
 template <>
 inline FunctionToLoopPassAdaptor
 createFunctionToLoopPassAdaptor<LoopPassManager>(LoopPassManager &&LPM,
                                                  bool UseMemorySSA,
                                                  bool UseBlockFrequencyInfo) {
   // Check if LPM contains any loop pass and if it does not, returns an adaptor
   // in loop-nest mode.
   using PassModelT =
       detail::PassModel<Loop, LoopPassManager, PreservedAnalyses,
                         LoopAnalysisManager, LoopStandardAnalysisResults &,
                         LPMUpdater &>;
   bool LoopNestMode = (LPM.getNumLoopPasses() == 0);
-  return FunctionToLoopPassAdaptor(std::make_unique<PassModelT>(std::move(LPM)),
-                                   UseMemorySSA, UseBlockFrequencyInfo,
-                                   LoopNestMode);
+  return FunctionToLoopPassAdaptor(
+      std::unique_ptr<FunctionToLoopPassAdaptor::PassConceptT>(
+          new PassModelT(std::move(LPM))),
+      UseMemorySSA, UseBlockFrequencyInfo, LoopNestMode);
 }
 
 /// Pass for printing a loop's contents as textual IR.
 class PrintLoopPass : public PassInfoMixin<PrintLoopPass> {
   raw_ostream &OS;
   std::string Banner;
 
 public: