Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NewPM][CodeGen] Refactoring CodeGenPassBuilder #89708

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paperchalice
Copy link
Contributor

@paperchalice paperchalice commented Apr 23, 2024

  • Remove redundant std::move.
  • Use a unified function to add passes.
  • Support pass substitution, i.e. user can specialize substitutePass to replace the pass by another pass. Currently PostRAScheduler could be substituted by PostMachineScheduler in some backends.
    Based on [NewPM][CodeGen] Add MachineFunctionAnalysis #88610.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 23, 2024

@llvm/pr-subscribers-backend-x86

Author: None (paperchalice)

Changes
  • Remove redundant std::move.
  • Use a unified function to add passes.
  • Support pass substitution.
    Based on #88610.

Patch is 47.68 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/89708.diff

4 Files Affected:

  • (modified) llvm/include/llvm/Passes/CodeGenPassBuilder.h (+286-264)
  • (modified) llvm/lib/Target/X86/X86CodeGenPassBuilder.cpp (+6-7)
  • (modified) llvm/unittests/CodeGen/CMakeLists.txt (+1)
  • (added) llvm/unittests/CodeGen/CodeGenPassBuilderTest.cpp (+126)
diff --git a/llvm/include/llvm/Passes/CodeGenPassBuilder.h b/llvm/include/llvm/Passes/CodeGenPassBuilder.h
index 2e94a19502131a..3c122d67eda1b7 100644
--- a/llvm/include/llvm/Passes/CodeGenPassBuilder.h
+++ b/llvm/include/llvm/Passes/CodeGenPassBuilder.h
@@ -72,6 +72,7 @@
 #include "llvm/Transforms/Utils/EntryExitInstrumenter.h"
 #include "llvm/Transforms/Utils/LowerInvoke.h"
 #include <cassert>
+#include <stack>
 #include <type_traits>
 #include <utility>
 
@@ -115,9 +116,6 @@ template <typename DerivedT, typename TargetMachineT> class CodeGenPassBuilder {
                               const CGPassBuilderOption &Opts,
                               PassInstrumentationCallbacks *PIC)
       : TM(TM), Opt(Opts), PIC(PIC) {
-    // Target could set CGPassBuilderOption::MISchedPostRA to true to achieve
-    //     substitutePass(&PostRASchedulerID, &PostMachineSchedulerID)
-
     // Target should override TM.Options.EnableIPRA in their target-specific
     // LLVMTM ctor. See TargetMachine::setGlobalISel for example.
     if (Opt.EnableIPRA)
@@ -131,142 +129,103 @@ template <typename DerivedT, typename TargetMachineT> class CodeGenPassBuilder {
   }
 
   Error buildPipeline(ModulePassManager &MPM, raw_pwrite_stream &Out,
-                      raw_pwrite_stream *DwoOut,
-                      CodeGenFileType FileType) const;
+                      raw_pwrite_stream *DwoOut, CodeGenFileType FileType);
 
-  PassInstrumentationCallbacks *getPassInstrumentationCallbacks() const {
+  PassInstrumentationCallbacks *getPassInstrumentationCallbacks() {
     return PIC;
   }
 
+  /// Add one pass to pass manager, it can handle pass nesting automatically.
+  template <typename PassT> void addPass(PassT &&Pass) {
+    using ResultT = decltype(derived().template substitutePass<PassT>());
+    constexpr bool IsVoid = std::is_void_v<ResultT>;
+    StringRef PassName = std::conditional_t<IsVoid, PassT, ResultT>::name();
+
+    if (!runBeforeAdding(PassName))
+      return;
+
+    if constexpr (IsVoid)
+      addPassImpl(std::forward<PassT>(Pass));
+    else
+      addPassImpl(derived().template substitutePass<PassT>());
+
+    runAfterAdding(PassName);
+  }
+
+  /// Add multiple passes.
+  template <typename PassT, typename... PassTs>
+  void addPass(PassT &&Pass, PassTs &&...Passes) {
+    addPass(std::forward<PassT>(Pass));
+    addPass(std::forward<PassTs>(Passes)...);
+  }
+
+  /// Add multiple passes with default constructor.
+  template <typename... PassTs> void addPass() { addPass(PassTs()...); }
+
+  /// @brief Substitute PassT with given pass, target can specialize this
+  /// function template or override it in subclass, if it is overriden by
+  /// subclass, it must return CodeGenPassBuilder::substitutePass() to get
+  /// default return value. See also
+  /// unittests/CodeGen/CodeGenPassBuilderTest.cpp.
+  /// @tparam PassT The pass type that needs to be replaced.
+  /// @return The replaced pass if substitution occurs, otherwise return void.
+  template <typename PassT> auto substitutePass() {}
+
 protected:
   template <typename PassT>
   using is_module_pass_t = decltype(std::declval<PassT &>().run(
       std::declval<Module &>(), std::declval<ModuleAnalysisManager &>()));
 
+  template <typename PassT>
+  static constexpr bool is_module_pass_v =
+      is_detected<is_module_pass_t, PassT>::value;
+
   template <typename PassT>
   using is_function_pass_t = decltype(std::declval<PassT &>().run(
       std::declval<Function &>(), std::declval<FunctionAnalysisManager &>()));
 
+  template <typename PassT>
+  static constexpr bool is_function_pass_v =
+      is_detected<is_function_pass_t, PassT>::value;
+
   template <typename PassT>
   using is_machine_function_pass_t = decltype(std::declval<PassT &>().run(
       std::declval<MachineFunction &>(),
       std::declval<MachineFunctionAnalysisManager &>()));
 
-  // Function object to maintain state while adding codegen IR passes.
-  // TODO: add a Function -> MachineFunction adaptor and merge
-  // AddIRPass/AddMachinePass so we can have a function pipeline that runs both
-  // function passes and machine function passes.
-  class AddIRPass {
-  public:
-    AddIRPass(ModulePassManager &MPM, const DerivedT &PB) : MPM(MPM), PB(PB) {}
-    ~AddIRPass() {
-      if (!FPM.isEmpty())
-        MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
-    }
-
-    template <typename PassT>
-    void operator()(PassT &&Pass, StringRef Name = PassT::name()) {
-      static_assert((is_detected<is_function_pass_t, PassT>::value ||
-                     is_detected<is_module_pass_t, PassT>::value) &&
-                    "Only module pass and function pass are supported.");
-
-      if (!PB.runBeforeAdding(Name))
-        return;
-
-      // Add Function Pass
-      if constexpr (is_detected<is_function_pass_t, PassT>::value) {
-        FPM.addPass(std::forward<PassT>(Pass));
-      } else {
-        // Add Module Pass
-        if (!FPM.isEmpty()) {
-          MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
-          FPM = FunctionPassManager();
-        }
-
-        MPM.addPass(std::forward<PassT>(Pass));
-      }
-    }
-
-  private:
-    ModulePassManager &MPM;
-    FunctionPassManager FPM;
-    const DerivedT &PB;
-  };
-
-  // Function object to maintain state while adding codegen machine passes.
-  class AddMachinePass {
-  public:
-    AddMachinePass(ModulePassManager &MPM, const DerivedT &PB)
-        : MPM(MPM), PB(PB) {}
-    ~AddMachinePass() {
-      if (!MFPM.isEmpty())
-        MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM)));
-    }
-
-    template <typename PassT>
-    void operator()(PassT &&Pass, bool Force = false,
-                    StringRef Name = PassT::name()) {
-      static_assert((is_detected<is_machine_function_pass_t, PassT>::value ||
-                     is_detected<is_module_pass_t, PassT>::value) &&
-                    "Only module pass and function pass are supported.");
-
-      if (!Force && !PB.runBeforeAdding(Name))
-        return;
-
-      // Add Function Pass
-      if constexpr (is_detected<is_machine_function_pass_t, PassT>::value) {
-        MFPM.addPass(std::forward<PassT>(Pass));
-      } else {
-        // Add Module Pass
-        if (!MFPM.isEmpty()) {
-          MPM.addPass(
-              createModuleToMachineFunctionPassAdaptor(std::move(MFPM)));
-          MFPM = MachineFunctionPassManager();
-        }
-
-        MPM.addPass(std::forward<PassT>(Pass));
-      }
-
-      for (auto &C : PB.AfterCallbacks)
-        C(Name, MFPM);
-    }
-
-  private:
-    ModulePassManager &MPM;
-    MachineFunctionPassManager MFPM;
-    const DerivedT &PB;
-  };
+  template <typename PassT>
+  static constexpr bool is_machine_function_pass_v =
+      is_detected<is_machine_function_pass_t, PassT>::value;
 
   TargetMachineT &TM;
   CGPassBuilderOption Opt;
   PassInstrumentationCallbacks *PIC;
 
-  template <typename TMC> TMC &getTM() const { return static_cast<TMC &>(TM); }
-  CodeGenOptLevel getOptLevel() const { return TM.getOptLevel(); }
+  CodeGenOptLevel getOptLevel() { return TM.getOptLevel(); }
 
   /// Check whether or not GlobalISel should abort on error.
   /// When this is disabled, GlobalISel will fall back on SDISel instead of
   /// erroring out.
-  bool isGlobalISelAbortEnabled() const {
+  bool isGlobalISelAbortEnabled() {
     return TM.Options.GlobalISelAbort == GlobalISelAbortMode::Enable;
   }
 
   /// Check whether or not a diagnostic should be emitted when GlobalISel
   /// uses the fallback path. In other words, it will emit a diagnostic
   /// when GlobalISel failed and isGlobalISelAbortEnabled is false.
-  bool reportDiagnosticWhenGlobalISelFallback() const {
+  bool reportDiagnosticWhenGlobalISelFallback() {
     return TM.Options.GlobalISelAbort == GlobalISelAbortMode::DisableWithDiag;
   }
 
   /// addInstSelector - This method should install an instruction selector pass,
   /// which converts from LLVM code to machine instructions.
-  Error addInstSelector(AddMachinePass &) const {
+  Error addInstSelector() {
     return make_error<StringError>("addInstSelector is not overridden",
                                    inconvertibleErrorCode());
   }
 
   /// Target can override this to add GlobalMergePass before all IR passes.
-  void addGlobalMergePass(AddIRPass &) const {}
+  void addGlobalMergePass() {}
 
   /// Add passes that optimize instruction level parallelism for out-of-order
   /// targets. These passes are run while the machine code is still in SSA
@@ -274,11 +233,11 @@ template <typename DerivedT, typename TargetMachineT> class CodeGenPassBuilder {
   ///
   /// All passes added here should preserve the MachineDominatorTree,
   /// MachineLoopInfo, and MachineTraceMetrics analyses.
-  void addILPOpts(AddMachinePass &) const {}
+  void addILPOpts() {}
 
   /// This method may be implemented by targets that want to run passes
   /// immediately before register allocation.
-  void addPreRegAlloc(AddMachinePass &) const {}
+  void addPreRegAlloc() {}
 
   /// addPreRewrite - Add passes to the optimized register allocation pipeline
   /// after register allocation is complete, but before virtual registers are
@@ -292,79 +251,77 @@ template <typename DerivedT, typename TargetMachineT> class CodeGenPassBuilder {
   /// Note if the target overloads addRegAssignAndRewriteOptimized, this may not
   /// be honored. This is also not generally used for the fast variant,
   /// where the allocation and rewriting are done in one pass.
-  void addPreRewrite(AddMachinePass &) const {}
+  void addPreRewrite() {}
 
   /// Add passes to be run immediately after virtual registers are rewritten
   /// to physical registers.
-  void addPostRewrite(AddMachinePass &) const {}
+  void addPostRewrite() {}
 
   /// This method may be implemented by targets that want to run passes after
   /// register allocation pass pipeline but before prolog-epilog insertion.
-  void addPostRegAlloc(AddMachinePass &) const {}
+  void addPostRegAlloc() {}
 
   /// This method may be implemented by targets that want to run passes after
   /// prolog-epilog insertion and before the second instruction scheduling pass.
-  void addPreSched2(AddMachinePass &) const {}
+  void addPreSched2() {}
 
   /// This pass may be implemented by targets that want to run passes
   /// immediately before machine code is emitted.
-  void addPreEmitPass(AddMachinePass &) const {}
+  void addPreEmitPass() {}
 
   /// Targets may add passes immediately before machine code is emitted in this
   /// callback. This is called even later than `addPreEmitPass`.
   // FIXME: Rename `addPreEmitPass` to something more sensible given its actual
   // position and remove the `2` suffix here as this callback is what
   // `addPreEmitPass` *should* be but in reality isn't.
-  void addPreEmitPass2(AddMachinePass &) const {}
+  void addPreEmitPass2() {}
 
   /// {{@ For GlobalISel
   ///
 
   /// addPreISel - This method should add any "last minute" LLVM->LLVM
   /// passes (which are run just before instruction selector).
-  void addPreISel(AddIRPass &) const {
-    llvm_unreachable("addPreISel is not overridden");
-  }
+  void addPreISel() { llvm_unreachable("addPreISel is not overridden"); }
 
   /// This method should install an IR translator pass, which converts from
   /// LLVM code to machine instructions with possibly generic opcodes.
-  Error addIRTranslator(AddMachinePass &) const {
+  Error addIRTranslator() {
     return make_error<StringError>("addIRTranslator is not overridden",
                                    inconvertibleErrorCode());
   }
 
   /// This method may be implemented by targets that want to run passes
   /// immediately before legalization.
-  void addPreLegalizeMachineIR(AddMachinePass &) const {}
+  void addPreLegalizeMachineIR() {}
 
   /// This method should install a legalize pass, which converts the instruction
   /// sequence into one that can be selected by the target.
-  Error addLegalizeMachineIR(AddMachinePass &) const {
+  Error addLegalizeMachineIR() {
     return make_error<StringError>("addLegalizeMachineIR is not overridden",
                                    inconvertibleErrorCode());
   }
 
   /// This method may be implemented by targets that want to run passes
   /// immediately before the register bank selection.
-  void addPreRegBankSelect(AddMachinePass &) const {}
+  void addPreRegBankSelect() {}
 
   /// This method should install a register bank selector pass, which
   /// assigns register banks to virtual registers without a register
   /// class or register banks.
-  Error addRegBankSelect(AddMachinePass &) const {
+  Error addRegBankSelect() {
     return make_error<StringError>("addRegBankSelect is not overridden",
                                    inconvertibleErrorCode());
   }
 
   /// This method may be implemented by targets that want to run passes
   /// immediately before the (global) instruction selection.
-  void addPreGlobalInstructionSelect(AddMachinePass &) const {}
+  void addPreGlobalInstructionSelect() {}
 
   /// This method should install a (global) instruction selector pass, which
   /// converts possibly generic instructions to fully target-specific
   /// instructions, thereby constraining all generic virtual registers to
   /// register classes.
-  Error addGlobalInstructionSelect(AddMachinePass &) const {
+  Error addGlobalInstructionSelect() {
     return make_error<StringError>(
         "addGlobalInstructionSelect is not overridden",
         inconvertibleErrorCode());
@@ -375,30 +332,30 @@ template <typename DerivedT, typename TargetMachineT> class CodeGenPassBuilder {
   /// representation to the MI representation.
   /// Adds IR based lowering and target specific optimization passes and finally
   /// the core instruction selection passes.
-  void addISelPasses(AddIRPass &) const;
+  void addISelPasses();
 
   /// Add the actual instruction selection passes. This does not include
   /// preparation passes on IR.
-  Error addCoreISelPasses(AddMachinePass &) const;
+  Error addCoreISelPasses();
 
   /// Add the complete, standard set of LLVM CodeGen passes.
   /// Fully developed targets will not generally override this.
-  Error addMachinePasses(AddMachinePass &) const;
+  Error addMachinePasses();
 
   /// Add passes to lower exception handling for the code generator.
-  void addPassesToHandleExceptions(AddIRPass &) const;
+  void addPassesToHandleExceptions();
 
   /// Add common target configurable passes that perform LLVM IR to IR
   /// transforms following machine independent optimization.
-  void addIRPasses(AddIRPass &) const;
+  void addIRPasses();
 
   /// Add pass to prepare the LLVM IR for code generation. This should be done
   /// before exception handling preparation passes.
-  void addCodeGenPrepare(AddIRPass &) const;
+  void addCodeGenPrepare();
 
   /// Add common passes that perform LLVM IR to IR transforms in preparation for
   /// instruction selection.
-  void addISelPrepare(AddIRPass &) const;
+  void addISelPrepare();
 
   /// Methods with trivial inline returns are convenient points in the common
   /// codegen pass pipeline where targets may insert passes. Methods with
@@ -409,30 +366,30 @@ template <typename DerivedT, typename TargetMachineT> class CodeGenPassBuilder {
 
   /// addMachineSSAOptimization - Add standard passes that optimize machine
   /// instructions in SSA form.
-  void addMachineSSAOptimization(AddMachinePass &) const;
+  void addMachineSSAOptimization();
 
   /// addFastRegAlloc - Add the minimum set of target-independent passes that
   /// are required for fast register allocation.
-  Error addFastRegAlloc(AddMachinePass &) const;
+  Error addFastRegAlloc();
 
   /// addOptimizedRegAlloc - Add passes related to register allocation.
   /// LLVMTargetMachine provides standard regalloc passes for most targets.
-  void addOptimizedRegAlloc(AddMachinePass &) const;
+  void addOptimizedRegAlloc();
 
   /// Add passes that optimize machine instructions after register allocation.
-  void addMachineLateOptimization(AddMachinePass &) const;
+  void addMachineLateOptimization();
 
   /// addGCPasses - Add late codegen passes that analyze code for garbage
   /// collection. This should return true if GC info should be printed after
   /// these passes.
-  void addGCPasses(AddMachinePass &) const {}
+  void addGCPasses() {}
 
   /// Add standard basic block placement passes.
-  void addBlockPlacement(AddMachinePass &) const;
+  void addBlockPlacement();
 
   using CreateMCStreamer =
       std::function<Expected<std::unique_ptr<MCStreamer>>(MCContext &)>;
-  void addAsmPrinter(AddMachinePass &, CreateMCStreamer) const {
+  void addAsmPrinter(CreateMCStreamer) {
     llvm_unreachable("addAsmPrinter is not overridden");
   }
 
@@ -441,67 +398,161 @@ template <typename DerivedT, typename TargetMachineT> class CodeGenPassBuilder {
 
   /// createTargetRegisterAllocator - Create the register allocator pass for
   /// this target at the current optimization level.
-  void addTargetRegisterAllocator(AddMachinePass &, bool Optimized) const;
+  void addTargetRegisterAllocator(bool Optimized);
 
   /// addMachinePasses helper to create the target-selected or overriden
   /// regalloc pass.
-  void addRegAllocPass(AddMachinePass &, bool Optimized) const;
+  void addRegAllocPass(bool Optimized);
 
   /// Add core register alloator passes which do the actual register assignment
   /// and rewriting. \returns true if any passes were added.
-  Error addRegAssignmentFast(AddMachinePass &) const;
-  Error addRegAssignmentOptimized(AddMachinePass &) const;
+  Error addRegAssignmentFast();
+  Error addRegAssignmentOptimized();
+
+  /// Merge all pass manager into one ModulePassManager
+  void mergePassManager();
 
   /// Allow the target to disable a specific pass by default.
   /// Backend can declare unwanted passes in constructor.
-  template <typename... PassTs> void disablePass() {
+  template <typename PassT> void disablePass(unsigned InstanceNum = 0) {
     BeforeCallbacks.emplace_back(
-        [](StringRef Name) { return ((Name != PassTs::name()) && ...); });
+        [Cnt = 0u, InstanceNum](StringRef Name) mutable {
+          if (!InstanceNum)
+            return PassT::name() != Name;
+          if (PassT::name() == Name)
+            return ++Cnt == InstanceNum;
+          return true;
+        });
+  }
+  template <typename PassT1, typename PassT2, typename... PassTs>
+  void disablePass() {
+    BeforeCallbacks.emplace_back([](StringRef Name) {
+      return Name != PassT1::name() && Name != PassT2::name() &&
+             ((Name != PassTs::name()) && ...);
+    });
   }
 
   /// Insert InsertedPass pass after TargetPass pass.
   /// Only machine function passes are supported.
   template <typename TargetPassT, typename InsertedPassT>
-  void insertPass(InsertedPassT &&Pass) {
-    AfterCallbacks.emplace_back(
-        [&](StringRef Name, MachineFunctionPassManager &MFPM) mutable {
-          if (Name == TargetPassT::name())
-            MFPM.addPass(std::forward<InsertedPassT>(Pass));
-        });
+  void insertPass(InsertedPassT &&Pass, unsigned InstanceNum = 0) {
+    AfterCallbacks.emplace_back([&, Cnt = 0](StringRef Name) mutable {
+      if (Name == TargetPassT::name()) {
+        if (!InstanceNum) {
+          addPass(std::forward<InsertedPassT>(Pass));
+          return;
+        }
+        if (++Cnt == InstanceNum)
+          addPass(std::forward<InsertedPassT>(Pass));
+      }
+    });
   }
 
+  /// Get internal ModulePassManager, only for test!
+  ModulePassManager &getMPM() { return MPM; }
+
 private:
   DerivedT &derived() { return static_cast<DerivedT &>(*this); }
-  const DerivedT &derived() const {
-    return static_cast<const DerivedT &>(*this);
-  }
 
-  bool runBeforeAdding(StringRef Name) const {
+  /// A monotonic stack based method to add pass.
+  template <typename PassT> void addPassImpl(PassT &&Pass);
+
+  bool runBeforeAdding(StringRef Name) {
     bool ShouldAdd = true;
     for (auto &C : BeforeCallbacks)
       ShouldAdd &= C(Name);
     return ShouldAdd;
   }
 
-  void setStartStopPasses(const TargetPassConfig::StartStopInfo &Info) const;
+  void runAfterAdding(StringRef Name) {
+    for (auto &C : AfterCallbacks)
+      C(Name);
+  }
+
+  void setStartStopPasses(const TargetPassConfig::StartStopInfo &Info);
 
   Error verifyStartStop(const TargetPassConf...
[truncated]

Copy link

github-actions bot commented Apr 23, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@paperchalice
Copy link
Contributor Author

I'd like to pull PassBuilder here, so user can write pass plugin for machine function.

@aeubanks
Copy link
Contributor

can you expand on the commit description? e.g. I'm not sure what "Support pass substitution" means.

btw, related to this, I was talking to Chandler about the new PM design, and one thing he was very insistent on was that we don't have some sort of automatic scheduling of module vs function passes, as we currently do in AddIRPass/AddMachinePass. this is because there have been cases in the past where it was very easy to accidentally add a module pass in the middle of a function pipeline without noticing. forcing the programmer to insert adaptors and break up function pipelines in makes it very clear what's going on.

this is of course much harder with the codegen pipeline than the optimization pipeline since each backend wants to customize it. the idea was to have a "default" pipeline that constructs the pipeline from various phases which backends can override. for example,

ModulePassManager buildPipeline() {
  ModulePassManager MPM;
  FunctionPassManager IRPasses = createIRPasses();
  MachineFunctionPassManager MIRPasses = createMIRPasses();
  IRPasses.addPass(createFunctionToMachineFunctionAdaptor(std::move(MIRPasses));
  MPM.addPass(createModuleToFunctionAdaptor(std::move(IRPasses));
  return MPM;
}

FunctionPassManager createIRPasses() {
  FunctionPassManager FPM;
  FPM.addPass(CodeGenPreparePass());
  ...
  return FPM;
}

MachineFunctionPassManager createMIRPasses() {
  MachineFunctionPassManager MFPM;
  MFPM.addPass(ISel());
  MFPM.addPass(createRegAllocPasses());
  ...
  return MFPM;
}

and any of these either smaller building blocks (e.g. createRegAllocPasses()), or the top level pipeline (which can use the smaller building blocks), are overridable by each backend

I'm unsure how feasible this actually is in practice though. having PassBuilder hooks to insert passes in specific parts of the pipeline would help with this though

@paperchalice
Copy link
Contributor Author

paperchalice commented Apr 25, 2024

can you expand on the commit description? e.g. I'm not sure what "Support pass substitution" means.

Some targets (ARM, RISCV etc.) and option --misched-postra require use PostMachineSchedulerPass rather PostRASchedulerPass, in fact, this is the only use of this function.

btw, related to this, I was talking to Chandler about the new PM design, and one thing he was very insistent on was that we don't have some sort of automatic scheduling of module vs function passes, as we currently do in AddIRPass/AddMachinePass. this is because there have been cases in the past where it was very easy to accidentally add a module pass in the middle of a function pipeline without noticing. forcing the programmer to insert adaptors and break up function pipelines in makes it very clear what's going on.

this is of course much harder with the codegen pipeline than the optimization pipeline since each backend wants to customize it. the idea was to have a "default" pipeline that constructs the pipeline from various phases which backends can override. for example,

ModulePassManager buildPipeline() {
  ModulePassManager MPM;
  FunctionPassManager IRPasses = createIRPasses();
  MachineFunctionPassManager MIRPasses = createMIRPasses();
  IRPasses.addPass(createFunctionToMachineFunctionAdaptor(std::move(MIRPasses));
  MPM.addPass(createModuleToFunctionAdaptor(std::move(IRPasses));
  return MPM;
}

FunctionPassManager createIRPasses() {
  FunctionPassManager FPM;
  FPM.addPass(CodeGenPreparePass());
  ...
  return FPM;
}

MachineFunctionPassManager createMIRPasses() {
  MachineFunctionPassManager MFPM;
  MFPM.addPass(ISel());
  MFPM.addPass(createRegAllocPasses());
  ...
  return MFPM;
}

and any of these either smaller building blocks (e.g. createRegAllocPasses()), or the top level pipeline (which can use the smaller building blocks), are overridable by each backend

I'm unsure how feasible this actually is in practice though. having PassBuilder hooks to insert passes in specific parts of the pipeline would help with this though

Oh, I see it. In fact I'm not sure whether IR pipelines in CodeGen pipelines are order sensitive for all targets, at least for now, some targets (like DirectX and AArch64) need to insert module passes around CodeGenPrepare and this part is order sensitive:

/// Add pass to prepare the LLVM IR for code generation. This should be done
/// before exception handling preparation passes.
void TargetPassConfig::addCodeGenPrepare() {
if (getOptLevel() != CodeGenOptLevel::None && !DisableCGP)
addPass(createCodeGenPrepareLegacyPass());
}

But provide pass insertion points in a finer granularity to ensure the module->function->machine-function->function->module pattern is better.


Update: Tried to rearrange some IR passes in AArch64PassConfig::addIRPasses (e.g. put createAtomicExpandLegacyPass after TargetPassConfig::addIRPasses), ~70 tests failed. Sometimes backends will intentionally break this pattern, another example is AArch64PassConfig::addISelPrepare which happens after addIRPasses:

// Run promote constant before global merge, so that the promoted constants
// get a chance to be merged
if (TM->getOptLevel() != CodeGenOptLevel::None && EnablePromoteConstant)
addPass(createAArch64PromoteConstantPass());

AArch64PromoteConstantPass is a module pass (In fact the comment here is invalid, GlobalMerge always happens before this pass, because GlobalMerge already does its job in doInitialization, this is another problem...)

@arsenm
Copy link
Contributor

arsenm commented Apr 25, 2024

can you expand on the commit description? e.g. I'm not sure what "Support pass substitution" means.

Some targets (ARM, RISCV etc.) and option --misched-postra require use PostMachineSchedulerPass rather PostRASchedulerPass, in fact, this is the only use of this function.

We should finally finish that migration...

@paperchalice
Copy link
Contributor Author

Pull PassBuilder so we can support machine function pass plugins.

@paperchalice
Copy link
Contributor Author

paperchalice commented May 5, 2024

Now derived class should use addModulePass, addFunctionPass, addMachineFunctionPass to add custom passes properly. Also remove some print and verify passes, because StandardInstrumentations supports machine functions now.

I recalled that ShadowStackGCLoweringPass is converted to module pass, because it indeed modified the module by adding some global variables and new pass manager is designed for future pass concurrency. It looks like it must happens after GCLoweringPass because it handles gc_root related initializers.

- Remove redundant `std::move`.
- Use a unified function to add passes.
- Support pass substitution.
@aeubanks
Copy link
Contributor

aeubanks commented May 7, 2024

sorry for the slow review on this, I've been trying to find time to write down my thoughts and understand the current codegen pipeline some more

all this removing and replacing of passes from a "default" codegen pipeline makes me nervous, especially with all the templatey stuff, plus being able to choose e.g. the third run of some pass. I'd really like to understand if we can do the "building blocks" approach above, perhaps with the help of some callbacks for targets to add passes into certain portions of the pipeline. IMO that seems more preferable and more understandable than a default pipeline that's completely customizable at every pass, which is hard to follow and doesn't seem principled

@arsenm thoughts?

@arsenm
Copy link
Contributor

arsenm commented May 7, 2024

I'd really like to understand if we can do the "building blocks" approach above, perhaps with the help of some callbacks for targets to add passes into certain portions of the pipeline. IMO that seems more preferable and more understandable than a default pipeline that's completely customizable at every pass, which is hard to follow and doesn't seem principled

We currently have both approaches in TargetPassConfig, which makes it difficult to follow where passes come from. We have addPass/insertPass/disablePass/substitutePass based on PassID, which are really hard to understand. We additionally have the assorted pipeline insertion point callbacks.

We actually call all the insertPass stuff inside the pipeline callbacks, but I don't think the point you call these actually matters which adds to the confusion. Overall I think it would be better to just have the assorted insertion point callbacks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants