[CodeGen][NewPM] Extract MachineFunctionProperties modification part to an RAII class

Modify MachineFunctionProperties in PassModel makes `PassT P; P.run(...);` not work properly.

Author: paperchalice

llvm/include/llvm/CodeGen/MachinePassManager.h

@@ -36,35 +36,20 @@ class MachineFunction;
 extern template class AnalysisManager<MachineFunction>;
 using MachineFunctionAnalysisManager = AnalysisManager<MachineFunction>;
 
-namespace detail {
-
-template <typename PassT>
-struct MachinePassModel
-    : PassModel<MachineFunction, PassT, MachineFunctionAnalysisManager> {
-  explicit MachinePassModel(PassT &&Pass)
-      : PassModel<MachineFunction, PassT, MachineFunctionAnalysisManager>(
-            std::move(Pass)) {}
-
-  friend void swap(MachinePassModel &LHS, MachinePassModel &RHS) {
-    using std::swap;
-    swap(LHS.Pass, RHS.Pass);
-  }
-
-  MachinePassModel &operator=(MachinePassModel RHS) {
-    swap(*this, RHS);
-    return *this;
-  }
-
-  MachinePassModel &operator=(const MachinePassModel &) = delete;
-  PreservedAnalyses run(MachineFunction &IR,
-                        MachineFunctionAnalysisManager &AM) override {
+/// An RAII based helper class to modify MachineFunctionProperties when running
+/// pass. Define a MFPropsModifier in PassT::run to set
+/// MachineFunctionProperties properly.
+template <typename PassT> class MFPropsModifier {
+public:
+  MFPropsModifier(PassT &P_, MachineFunction &MF_) : P(P_), MF(MF_) {
+    auto &MFProps = MF.getProperties();
 #ifndef NDEBUG
-    if constexpr (is_detected<has_get_required_properties_t, PassT>::value) {
-      auto &MFProps = IR.getProperties();
-      auto RequiredProperties = this->Pass.getRequiredProperties();
+    if constexpr (has_get_required_properties_v<PassT>) {
+      auto &MFProps = MF.getProperties();
+      auto RequiredProperties = P.getRequiredProperties();
       if (!MFProps.verifyRequiredProperties(RequiredProperties)) {
         errs() << "MachineFunctionProperties required by " << PassT::name()
-               << " pass are not met by function " << IR.getName() << ".\n"
+               << " pass are not met by function " << MF.getName() << ".\n"
                << "Required properties: ";
         RequiredProperties.print(errs());
         errs() << "\nCurrent properties: ";
@@ -73,18 +58,22 @@ struct MachinePassModel
         report_fatal_error("MachineFunctionProperties check failed");
       }
     }
-#endif
-
-    auto PA = this->Pass.run(IR, AM);
+#endif // NDEBUG
+    if constexpr (has_get_cleared_properties_v<PassT>)
+      MFProps.reset(P.getClearedProperties());
+  }
 
-    if constexpr (is_detected<has_get_set_properties_t, PassT>::value)
-      IR.getProperties().set(this->Pass.getSetProperties());
-    if constexpr (is_detected<has_get_cleared_properties_t, PassT>::value)
-      IR.getProperties().reset(this->Pass.getClearedProperties());
-    return PA;
+  ~MFPropsModifier() {
+    if constexpr (has_get_set_properties_v<PassT>) {
+      auto &MFProps = MF.getProperties();
+      MFProps.set(P.getSetProperties());
+    }
   }
 
 private:
+  PassT &P;
+  MachineFunction &MF;
+
   template <typename T>
   using has_get_required_properties_t =
       decltype(std::declval<T &>().getRequiredProperties());
@@ -96,8 +85,19 @@ struct MachinePassModel
   template <typename T>
   using has_get_cleared_properties_t =
       decltype(std::declval<T &>().getClearedProperties());
+
+  template <typename T>
+  static constexpr bool has_get_required_properties_v =
+      is_detected<has_get_required_properties_t, T>::value;
+
+  template <typename T>
+  static constexpr bool has_get_set_properties_v =
+      is_detected<has_get_set_properties_t, T>::value;
+
+  template <typename T>
+  static constexpr bool has_get_cleared_properties_v =
+      is_detected<has_get_cleared_properties_t, T>::value;
 };
-} // namespace detail
 
 using MachineFunctionAnalysisManagerModuleProxy =
     InnerAnalysisManagerProxy<MachineFunctionAnalysisManager, Module>;
@@ -219,21 +219,6 @@ createFunctionToMachineFunctionPassAdaptor(MachineFunctionPassT &&Pass) {
           new PassModelT(std::forward<MachineFunctionPassT>(Pass))));
 }
 
-template <>
-template <typename PassT>
-void PassManager<MachineFunction>::addPass(PassT &&Pass) {
-  using MachinePassModelT = detail::MachinePassModel<PassT>;
-  // Do not use make_unique or emplace_back, they cause too many template
-  // instantiations, causing terrible compile times.
-  if constexpr (std::is_same_v<PassT, PassManager<MachineFunction>>) {
-    for (auto &P : Pass.Passes)
-      Passes.push_back(std::move(P));
-  } else {
-    Passes.push_back(std::unique_ptr<MachinePassModelT>(
-        new MachinePassModelT(std::forward<PassT>(Pass))));
-  }
-}
-
 template <>
 PreservedAnalyses
 PassManager<MachineFunction>::run(MachineFunction &,

llvm/include/llvm/IR/PassManager.h

@@ -245,24 +245,27 @@ class PassManager : public PassInfoMixin<
     return PA;
   }
 
-  // FIXME: Revert to enable_if style when gcc >= 11.1
-  template <typename PassT> LLVM_ATTRIBUTE_MINSIZE void addPass(PassT &&Pass) {
+  template <typename PassT>
+  LLVM_ATTRIBUTE_MINSIZE std::enable_if_t<!std::is_same_v<PassT, PassManager>>
+  addPass(PassT &&Pass) {
     using PassModelT =
         detail::PassModel<IRUnitT, PassT, AnalysisManagerT, ExtraArgTs...>;
-    if constexpr (!std::is_same_v<PassT, PassManager>) {
-      // Do not use make_unique or emplace_back, they cause too many template
-      // instantiations, causing terrible compile times.
-      Passes.push_back(std::unique_ptr<PassConceptT>(
-          new PassModelT(std::forward<PassT>(Pass))));
-    } else {
-      /// 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.
-      for (auto &P : Pass.Passes)
-        Passes.push_back(std::move(P));
-    }
+    // Do not use make_unique or emplace_back, they cause too many template
+    // instantiations, causing terrible compile times.
+    Passes.push_back(std::unique_ptr<PassConceptT>(
+        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>
+  LLVM_ATTRIBUTE_MINSIZE std::enable_if_t<std::is_same_v<PassT, PassManager>>
+  addPass(PassT &&Pass) {
+    for (auto &P : Pass.Passes)
+      Passes.push_back(std::move(P));
   }
 
   /// Returns if the pass manager contains any passes.

llvm/lib/CodeGen/RegAllocFast.cpp

@@ -1790,6 +1790,7 @@ bool RegAllocFastImpl::runOnMachineFunction(MachineFunction &MF) {
 
 PreservedAnalyses RegAllocFastPass::run(MachineFunction &MF,
                                         MachineFunctionAnalysisManager &) {
+  MFPropsModifier _(*this, MF);
   RegAllocFastImpl Impl(Opts.Filter, Opts.ClearVRegs);
   bool Changed = Impl.runOnMachineFunction(MF);
   if (!Changed)

llvm/lib/Passes/PassBuilder.cpp

@@ -385,7 +385,8 @@ class TriggerVerifierErrorPass
 class RequireAllMachineFunctionPropertiesPass
     : public PassInfoMixin<RequireAllMachineFunctionPropertiesPass> {
 public:
-  PreservedAnalyses run(MachineFunction &, MachineFunctionAnalysisManager &) {
+  PreservedAnalyses run(MachineFunction &MF, MachineFunctionAnalysisManager &) {
+    MFPropsModifier _(*this, MF);
     return PreservedAnalyses::none();
   }