[NewPM] Fix wrong perfect forwardings

[vhscampos]

Summary

Some template functions were missing '&&' in function arguments,
therefore these were always taken by value after template instantiation.

This patch adds the double ampersand to introduce proper perfect
forwarding.

[aeubanks]

there are a couple more adaptors in CGSCCPassManager.h and LoopPassManager.h, should those be udpated too?

[vhscampos]

Fixes in other places of the new pass manager.

[vhscampos]

@aeubanks Yes, thank you. Just did that.

[aeubanks]

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

136

forward?

148

forward?

490

forward?

[vhscampos]

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

136

Pass is an rvalue reference.

Note that the type of 'Pass' is RepeatedPass<PassT>, not PassT, i.e. it does not have the type of the template. Which means that perfect forwarding doesn't work here. Therefore we'd need one implementation for lvalue ref and another for rvalue ref. Since this function is called only passing rvalues, it is safe to have only the rvalue reference version.

148

Same reason as above.

490

In this case here, LPM is fixed as an rvalue reference, so no need for forwarding. That means that it accepts only calls that pass an rvalue. This is true for all call sites.

[aeubanks]

lgtm

[vhscampos]

Thanks for your review

llvm/include/llvm/Analysis/CGSCCPassManager.h

 private:
   std::unique_ptr<PassConceptT> Pass;
 };
 
 /// A function to deduce a function pass type and wrap it in the
 /// templated adaptor.
 template <typename CGSCCPassT>
 ModuleToPostOrderCGSCCPassAdaptor
-createModuleToPostOrderCGSCCPassAdaptor(CGSCCPassT Pass) {
+createModuleToPostOrderCGSCCPassAdaptor(CGSCCPassT &&Pass) {
   using PassModelT = detail::PassModel<LazyCallGraph::SCC, CGSCCPassT,
                                        PreservedAnalyses, CGSCCAnalysisManager,
                                        LazyCallGraph &, CGSCCUpdateResult &>;
   return ModuleToPostOrderCGSCCPassAdaptor(
-      std::make_unique<PassModelT>(std::move(Pass)));
+      std::make_unique<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

 private:
   std::unique_ptr<PassConceptT> Pass;
 };
 
 /// A function to deduce a function pass type and wrap it in the
 /// templated adaptor.
 template <typename FunctionPassT>
 CGSCCToFunctionPassAdaptor
-createCGSCCToFunctionPassAdaptor(FunctionPassT Pass) {
+createCGSCCToFunctionPassAdaptor(FunctionPassT &&Pass) {
   using PassModelT =
       detail::PassModel<Function, FunctionPassT, PreservedAnalyses,
                         FunctionAnalysisManager>;
   return CGSCCToFunctionPassAdaptor(
-      std::make_unique<PassModelT>(std::move(Pass)));
+      std::make_unique<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

 private:
   std::unique_ptr<PassConceptT> Pass;
   int MaxIterations;
 };
 
 /// A function to deduce a function pass type and wrap it in the
 /// templated adaptor.
 template <typename CGSCCPassT>
-DevirtSCCRepeatedPass createDevirtSCCRepeatedPass(CGSCCPassT Pass,
+DevirtSCCRepeatedPass createDevirtSCCRepeatedPass(CGSCCPassT &&Pass,
                                                   int MaxIterations) {
   using PassModelT = detail::PassModel<LazyCallGraph::SCC, CGSCCPassT,
                                        PreservedAnalyses, CGSCCAnalysisManager,
                                        LazyCallGraph &, CGSCCUpdateResult &>;
-  return DevirtSCCRepeatedPass(std::make_unique<PassModelT>(std::move(Pass)),
-                               MaxIterations);
+  return DevirtSCCRepeatedPass(
+      std::make_unique<PassModelT>(std::forward<CGSCCPassT>(Pass)),
 }
 
 // Clear out the debug logging macro.
 #undef DEBUG_TYPE
 
 } // end namespace llvm
 
 #endif // LLVM_ANALYSIS_CGSCCPASSMANAGER_H

llvm/include/llvm/IR/PassManager.h

PreservedAnalyses run(IRUnitT &IR, AnalysisManagerT &AM,
     // need to inspect each one individually.
     PA.preserveSet<AllAnalysesOn<IRUnitT>>();
 
     return PA;
   }
 
   template <typename PassT>
   std::enable_if_t<!std::is_same<PassT, PassManager>::value>
-  addPass(PassT Pass) {
+  addPass(PassT &&Pass) {
     using PassModelT =
         detail::PassModel<IRUnitT, PassT, PreservedAnalyses, AnalysisManagerT,
                           ExtraArgTs...>;
 
-    Passes.emplace_back(new PassModelT(std::move(Pass)));
+    Passes.emplace_back(new PassModelT(std::forward<PassT>(Pass)));
   }
 
   /// When adding a pass manager pass that has the same type as this pass
   /// manager, simply move the passes over. This is because we don't have use
   /// cases rely on executing nested pass managers. Doing this could reduce
   /// implementation complexity and avoid potential invalidation issues that may
   /// happen with nested pass managers of the same type.
   template <typename PassT>

 private:
   std::unique_ptr<PassConceptT> Pass;
 };
 
 /// A function to deduce a function pass type and wrap it in the
 /// templated adaptor.
 template <typename FunctionPassT>
 ModuleToFunctionPassAdaptor
-createModuleToFunctionPassAdaptor(FunctionPassT Pass) {
+createModuleToFunctionPassAdaptor(FunctionPassT &&Pass) {
   using PassModelT =
       detail::PassModel<Function, FunctionPassT, PreservedAnalyses,
                         FunctionAnalysisManager>;
 
   return ModuleToFunctionPassAdaptor(
-      std::make_unique<PassModelT>(std::move(Pass)));
+      std::make_unique<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.

 
 /// A utility pass template that simply runs another pass multiple times.
 ///
 /// This can be useful when debugging or testing passes. It also serves as an
 /// example of how to extend the pass manager in ways beyond composition.
 template <typename PassT>
 class RepeatedPass : public PassInfoMixin<RepeatedPass<PassT>> {
 public:
-  RepeatedPass(int Count, PassT P) : Count(Count), P(std::move(P)) {}
+  RepeatedPass(int Count, PassT &&P)
+      : Count(Count), P(std::forward<PassT>(P)) {}
 
   template <typename IRUnitT, typename AnalysisManagerT, typename... Ts>
   PreservedAnalyses run(IRUnitT &IR, AnalysisManagerT &AM, Ts &&... Args) {
 
     // Request PassInstrumentation from analysis manager, will use it to run
     // instrumenting callbacks for the passes later.
     // Here we use std::tuple wrapper over getResult which helps to extract
     // AnalysisManager's arguments out of the whole Args set.
public:
   }
 
 private:
   int Count;
   PassT P;
 };
 
 template <typename PassT>
-RepeatedPass<PassT> createRepeatedPass(int Count, PassT P) {
-  return RepeatedPass<PassT>(Count, std::move(P));
+RepeatedPass<PassT> createRepeatedPass(int Count, PassT &&P) {
+  return RepeatedPass<PassT>(Count, std::forward<PassT>(P));
 }
 
 } // end namespace llvm
 
 #endif // LLVM_IR_PASSMANAGER_H

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

public:
   /// Add either a loop pass or a loop-nest pass to the pass manager. Append \p
   /// Pass to the list of loop passes if it has a dedicated \fn run() method for
   /// loops and to the list of loop-nest passes if the \fn run() method is for
   /// loop-nests instead. Also append whether \p Pass is loop-nest pass or not
   /// to the end of \var IsLoopNestPass so we can easily identify the types of
   /// passes in the pass manager later.
   template <typename PassT>
   std::enable_if_t<is_detected<HasRunOnLoopT, PassT>::value>
-  addPass(PassT Pass) {
+  addPass(PassT &&Pass) {
     using LoopPassModelT =
         detail::PassModel<Loop, PassT, PreservedAnalyses, LoopAnalysisManager,
                           LoopStandardAnalysisResults &, LPMUpdater &>;
     IsLoopNestPass.push_back(false);
-    LoopPasses.emplace_back(new LoopPassModelT(std::move(Pass)));
+    LoopPasses.emplace_back(new LoopPassModelT(std::forward<PassT>(Pass)));
   }
 
   template <typename PassT>
   std::enable_if_t<!is_detected<HasRunOnLoopT, PassT>::value>
-  addPass(PassT Pass) {
+  addPass(PassT &&Pass) {
     using LoopNestPassModelT =
         detail::PassModel<LoopNest, PassT, PreservedAnalyses,
                           LoopAnalysisManager, LoopStandardAnalysisResults &,
                           LPMUpdater &>;
     IsLoopNestPass.push_back(true);
-    LoopNestPasses.emplace_back(new LoopNestPassModelT(std::move(Pass)));
+    LoopNestPasses.emplace_back(
+        new LoopNestPassModelT(std::forward<PassT>(Pass)));
   }
 
   // Specializations of `addPass` for `RepeatedPass`. These are necessary since
   // `RepeatedPass` has a templated `run` method that will result in incorrect
   // detection of `HasRunOnLoopT`.
   template <typename PassT>
   std::enable_if_t<is_detected<HasRunOnLoopT, PassT>::value>
-  addPass(RepeatedPass<PassT> Pass) {
+  addPass(RepeatedPass<PassT> &&Pass) {
     using RepeatedLoopPassModelT =
         detail::PassModel<Loop, RepeatedPass<PassT>, PreservedAnalyses,
                           LoopAnalysisManager, LoopStandardAnalysisResults &,
                           LPMUpdater &>;
     IsLoopNestPass.push_back(false);
     LoopPasses.emplace_back(new RepeatedLoopPassModelT(std::move(Pass)));
   }
 
   template <typename PassT>
   std::enable_if_t<!is_detected<HasRunOnLoopT, PassT>::value>
-  addPass(RepeatedPass<PassT> Pass) {
+  addPass(RepeatedPass<PassT> &&Pass) {
     using RepeatedLoopNestPassModelT =
         detail::PassModel<LoopNest, RepeatedPass<PassT>, PreservedAnalyses,
                           LoopAnalysisManager, LoopStandardAnalysisResults &,
                           LPMUpdater &>;
     IsLoopNestPass.push_back(true);
     LoopNestPasses.emplace_back(
         new RepeatedLoopNestPassModelT(std::move(Pass)));
   }
 
   bool isEmpty() const { return LoopPasses.empty() && LoopNestPasses.empty(); }
 
   static bool isRequired() { return true; }
 
   size_t getNumLoopPasses() const { return LoopPasses.size(); }
   size_t getNumLoopNestPasses() const { return LoopNestPasses.size(); }

 
 /// A function to deduce a loop pass type and wrap it in the templated
 /// adaptor.
 ///
 /// If \p Pass is a loop pass, the returned adaptor will be in loop mode.
 template <typename LoopPassT>
 inline std::enable_if_t<is_detected<HasRunOnLoopT, LoopPassT>::value,
                         FunctionToLoopPassAdaptor>
-createFunctionToLoopPassAdaptor(LoopPassT Pass, bool UseMemorySSA = false,
+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::move(Pass)), UseMemorySSA,
+      std::make_unique<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,
+createFunctionToLoopPassAdaptor(LoopNestPassT &&Pass, bool UseMemorySSA = false,
                                 bool UseBlockFrequencyInfo = false) {
   LoopPassManager LPM;
-  LPM.addPass(std::move(Pass));
+  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);
 }
 
 /// 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,
+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);
 }
 
 /// Pass for printing a loop's contents as textual IR.
 class PrintLoopPass : public PassInfoMixin<PrintLoopPass> {
   raw_ostream &OS;
   std::string Banner;