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

paperchalice · 2024-06-08T12:49:42Z

Modify MachineFunctionProperties in PassModel makes PassT P; P.run(...); not work properly. This is a necessary compromise.

llvmbot · 2024-06-08T12:50:11Z

@llvm/pr-subscribers-llvm-ir

Author: None (paperchalice)

Changes

Modify MachineFunctionProperties in PassModel makes PassT P; P.run(...); not work properly. This is a necessary compromise.

Full diff: https://github.com/llvm/llvm-project/pull/94854.diff

4 Files Affected:

(modified) llvm/include/llvm/CodeGen/MachinePassManager.h (+31-56)
(modified) llvm/include/llvm/IR/PassManager.h (+20-16)
(modified) llvm/lib/CodeGen/RegAllocFast.cpp (+1)
(modified) llvm/lib/Passes/PassBuilder.cpp (+2-1)

diff --git a/llvm/include/llvm/CodeGen/MachinePassManager.h b/llvm/include/llvm/CodeGen/MachinePassManager.h
index 7d15664fbe754..0cf872e2aec8a 100644
--- a/llvm/include/llvm/CodeGen/MachinePassManager.h
+++ b/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,31 +58,36 @@ 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());
+  static constexpr bool has_get_required_properties_v =
+      is_detected<decltype(std::declval<T &>().getRequiredProperties()),
+                  T>::value;
 
   template <typename T>
-  using has_get_set_properties_t =
-      decltype(std::declval<T &>().getSetProperties());
+  static constexpr bool has_get_set_properties_v =
+      is_detected<decltype(std::declval<T &>().getSetProperties()), T>::value;
 
   template <typename T>
-  using has_get_cleared_properties_t =
-      decltype(std::declval<T &>().getClearedProperties());
+  static constexpr bool has_get_cleared_properties_v =
+      is_detected<decltype(std::declval<T &>().getClearedProperties()),
+                  T>::value;
 };
-} // namespace detail
 
 using MachineFunctionAnalysisManagerModuleProxy =
     InnerAnalysisManagerProxy<MachineFunctionAnalysisManager, Module>;
@@ -219,21 +209,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 &,
diff --git a/llvm/include/llvm/IR/PassManager.h b/llvm/include/llvm/IR/PassManager.h
index d701481202f8d..da25b8b495f89 100644
--- a/llvm/include/llvm/IR/PassManager.h
+++ b/llvm/include/llvm/IR/PassManager.h
@@ -245,24 +245,28 @@ 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<PassT, PassManager>::value>
+      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<PassT, PassManager>::value> {
+    for (auto &P : Pass.Passes)
+      Passes.push_back(std::move(P));
   }
 
   /// Returns if the pass manager contains any passes.
diff --git a/llvm/lib/CodeGen/RegAllocFast.cpp b/llvm/lib/CodeGen/RegAllocFast.cpp
index 09ce8c42a3850..f2854090bdd91 100644
--- a/llvm/lib/CodeGen/RegAllocFast.cpp
+++ b/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)
diff --git a/llvm/lib/Passes/PassBuilder.cpp b/llvm/lib/Passes/PassBuilder.cpp
index 2c56b04a1d9c8..7c7367db1c549 100644
--- a/llvm/lib/Passes/PassBuilder.cpp
+++ b/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();
   }

…to an RAII class Modify MachineFunctionProperties in PassModel makes `PassT P; P.run(...);` not work properly.

paperchalice · 2024-06-14T09:33:25Z

Ping? @aeubanks @arsenm

aeubanks

sorry for the slow review, I've been pretty swamped recently

lg with comment addressed, although I'll probably send out some followup changes at some point to simplify things even more

aeubanks · 2024-06-17T16:52:05Z

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


https://www.llvm.org/docs/GettingStarted.html#getting-a-modern-host-c-toolchain says we still support some gcc <11 (this should be a separate PR anyway)

This was introduced in b361b53 to distinguish machine function passes and ir passes. MachinePassModel is removed now, so we can revert to enable_if form.

ah sorry, I was mixing up enable_if_t and enable_if

llvm-ci · 2024-06-22T09:39:32Z

LLVM Buildbot has detected a new failure on builder clang-ppc64le-rhel running on ppc64le-clang-rhel-test while building llvm at step 5 "build-unified-tree".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/206