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] Let ModuleToMachineFunctionPassAdaptor own MachineModuleInfo #87226

Closed
wants to merge 1 commit into from

Conversation

paperchalice
Copy link
Contributor

@paperchalice paperchalice commented Apr 1, 2024

Thus MachineFunctions are no longer analysis results, but still keep MachineModuleAnalysis so "machine module pass" can work. The first ModuleToMachineFunctionPassAdaptor in pipeline owns all machine functions, and can obtain associated MachineModuleInfo from PassBuilder if the pipeline is built by PassBuilder.
See https://discourse.llvm.org/t/machinefunction-ownership-in-the-new-pass-manager/ and issue #84397 .

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 1, 2024

@llvm/pr-subscribers-backend-x86

Author: None (paperchalice)

Changes

Thus MachineFunctions are no longer analysis results, but still keep MachineModuleAnalysis so "machine module pass" can work. The first ModuleToMachineFunctionPassAdaptor in pipeline owns all machine functions, and can obtain associated MachineModuleInfo from PassBuilder if the pipeline is built by PassBuilder.


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

12 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/MachinePassManager.h (+32-4)
  • (modified) llvm/include/llvm/Passes/CodeGenPassBuilder.h (+22-8)
  • (modified) llvm/include/llvm/Passes/PassBuilder.h (+13)
  • (modified) llvm/include/llvm/Target/TargetMachine.h (+1-1)
  • (modified) llvm/lib/CodeGen/MachinePassManager.cpp (+1-1)
  • (modified) llvm/lib/Passes/PassBuilder.cpp (+16-3)
  • (modified) llvm/lib/Passes/PassRegistry.def (+1)
  • (modified) llvm/lib/Target/X86/X86CodeGenPassBuilder.cpp (+4-5)
  • (modified) llvm/lib/Target/X86/X86TargetMachine.h (+1-1)
  • (modified) llvm/tools/llc/NewPMDriver.cpp (+3-5)
  • (modified) llvm/unittests/CodeGen/PassManagerTest.cpp (+3-5)
  • (modified) llvm/unittests/MIR/PassBuilderCallbacksTest.cpp (+3-3)
diff --git a/llvm/include/llvm/CodeGen/MachinePassManager.h b/llvm/include/llvm/CodeGen/MachinePassManager.h
index 8689fd19030f90..ac24bac65f9143 100644
--- a/llvm/include/llvm/CodeGen/MachinePassManager.h
+++ b/llvm/include/llvm/CodeGen/MachinePassManager.h
@@ -24,6 +24,7 @@
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/PassManagerInternal.h"
 #include "llvm/Support/Error.h"
@@ -217,30 +218,57 @@ class ModuleToMachineFunctionPassAdaptor
       detail::PassConcept<MachineFunction, MachineFunctionAnalysisManager>;
 
   explicit ModuleToMachineFunctionPassAdaptor(
-      std::unique_ptr<PassConceptT> Pass)
-      : Pass(std::move(Pass)) {}
+      std::unique_ptr<PassConceptT> Pass,
+      std::unique_ptr<MachineModuleInfo> MMI)
+      : Pass(std::move(Pass)), OwnedMMI(std::move(MMI)) {}
+
+  explicit ModuleToMachineFunctionPassAdaptor(
+      std::unique_ptr<PassConceptT> Pass, MachineModuleInfo &MMI)
+      : Pass(std::move(Pass)), MMIPtr(&MMI) {}
 
   /// Runs the function pass across every function in the module.
   PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
   void printPipeline(raw_ostream &OS,
                      function_ref<StringRef(StringRef)> MapClassName2PassName);
 
+  MachineModuleInfo &getMachineModuleInfo() {
+    assert((OwnedMMI || MMIPtr) && "Need MachineModuleInfo!");
+    return OwnedMMI ? *OwnedMMI : *MMIPtr;
+  }
+
   static bool isRequired() { return true; }
 
 private:
   std::unique_ptr<PassConceptT> Pass;
+  std::unique_ptr<MachineModuleInfo> OwnedMMI;
+  MachineModuleInfo *MMIPtr = nullptr;
 };
 
+template <typename MachineFunctionPassT>
+ModuleToMachineFunctionPassAdaptor createModuleToMachineFunctionPassAdaptor(
+    MachineFunctionPassT &&Pass, std::unique_ptr<MachineModuleInfo> &&MMI) {
+  using PassModelT = detail::PassModel<MachineFunction, MachineFunctionPassT,
+                                       MachineFunctionAnalysisManager>;
+  // Do not use make_unique, it causes too many template instantiations,
+  // causing terrible compile times.
+  return ModuleToMachineFunctionPassAdaptor(
+      std::unique_ptr<ModuleToMachineFunctionPassAdaptor::PassConceptT>(
+          new PassModelT(std::forward<MachineFunctionPassT>(Pass))),
+      std::move(MMI));
+}
+
 template <typename MachineFunctionPassT>
 ModuleToMachineFunctionPassAdaptor
-createModuleToMachineFunctionPassAdaptor(MachineFunctionPassT &&Pass) {
+createModuleToMachineFunctionPassAdaptor(MachineFunctionPassT &&Pass,
+                                         MachineModuleInfo &MMI) {
   using PassModelT = detail::PassModel<MachineFunction, MachineFunctionPassT,
                                        MachineFunctionAnalysisManager>;
   // Do not use make_unique, it causes too many template instantiations,
   // causing terrible compile times.
   return ModuleToMachineFunctionPassAdaptor(
       std::unique_ptr<ModuleToMachineFunctionPassAdaptor::PassConceptT>(
-          new PassModelT(std::forward<MachineFunctionPassT>(Pass))));
+          new PassModelT(std::forward<MachineFunctionPassT>(Pass))),
+      MMI);
 }
 
 template <>
diff --git a/llvm/include/llvm/Passes/CodeGenPassBuilder.h b/llvm/include/llvm/Passes/CodeGenPassBuilder.h
index 00eb9b096a9356..2351f6ed4edb4e 100644
--- a/llvm/include/llvm/Passes/CodeGenPassBuilder.h
+++ b/llvm/include/llvm/Passes/CodeGenPassBuilder.h
@@ -55,6 +55,7 @@
 #include "llvm/IRPrinter/IRPrintingPasses.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCTargetOptions.h"
+#include "llvm/Passes/PassBuilder.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
@@ -112,9 +113,8 @@ namespace llvm {
 template <typename DerivedT> class CodeGenPassBuilder {
 public:
   explicit CodeGenPassBuilder(LLVMTargetMachine &TM,
-                              const CGPassBuilderOption &Opts,
-                              PassInstrumentationCallbacks *PIC)
-      : TM(TM), Opt(Opts), PIC(PIC) {
+                              const CGPassBuilderOption &Opts, PassBuilder &PB)
+      : TM(TM), Opt(Opts), PB(PB), PIC(PB.getPassInstrumentationCallbacks()) {
     // Target could set CGPassBuilderOption::MISchedPostRA to true to achieve
     //     substitutePass(&PostRASchedulerID, &PostMachineSchedulerID)
 
@@ -205,8 +205,15 @@ template <typename DerivedT> class CodeGenPassBuilder {
     AddMachinePass(ModulePassManager &MPM, const DerivedT &PB)
         : MPM(MPM), PB(PB) {}
     ~AddMachinePass() {
-      if (!MFPM.isEmpty())
-        MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM)));
+      if (!MFPM.isEmpty()) {
+        if (PB.PB.hasMachineModuleInfoOwnership()) {
+          MPM.addPass(createModuleToMachineFunctionPassAdaptor(
+              std::move(MFPM), std::move(PB.PB.takeMachineModuleInfo())));
+        } else {
+          MPM.addPass(createModuleToMachineFunctionPassAdaptor(
+              std::move(MFPM), PB.PB.getMachineModuleInfo()));
+        }
+      }
     }
 
     template <typename PassT>
@@ -228,8 +235,13 @@ template <typename DerivedT> class CodeGenPassBuilder {
       } else {
         // Add Module Pass
         if (!MFPM.isEmpty()) {
-          MPM.addPass(
-              createModuleToMachineFunctionPassAdaptor(std::move(MFPM)));
+          if (PB.PB.hasMachineModuleInfoOwnership()) {
+            MPM.addPass(createModuleToMachineFunctionPassAdaptor(
+                std::move(MFPM), PB.PB.takeMachineModuleInfo()));
+          } else {
+            MPM.addPass(createModuleToMachineFunctionPassAdaptor(
+                std::move(MFPM), PB.PB.getMachineModuleInfo()));
+          }
           MFPM = MachineFunctionPassManager();
         }
 
@@ -248,6 +260,7 @@ template <typename DerivedT> class CodeGenPassBuilder {
 
   LLVMTargetMachine &TM;
   CGPassBuilderOption Opt;
+  PassBuilder &PB;
   PassInstrumentationCallbacks *PIC;
 
   template <typename TMC> TMC &getTM() const { return static_cast<TMC &>(TM); }
@@ -491,7 +504,8 @@ template <typename Derived>
 Error CodeGenPassBuilder<Derived>::buildPipeline(
     ModulePassManager &MPM, raw_pwrite_stream &Out, raw_pwrite_stream *DwoOut,
     CodeGenFileType FileType) const {
-  auto StartStopInfo = TargetPassConfig::getStartStopInfo(*PIC);
+  auto StartStopInfo =
+      TargetPassConfig::getStartStopInfo(*PB.getPassInstrumentationCallbacks());
   if (!StartStopInfo)
     return StartStopInfo.takeError();
   setStartStopPasses(*StartStopInfo);
diff --git a/llvm/include/llvm/Passes/PassBuilder.h b/llvm/include/llvm/Passes/PassBuilder.h
index d1232124d5d819..754797ad378376 100644
--- a/llvm/include/llvm/Passes/PassBuilder.h
+++ b/llvm/include/llvm/Passes/PassBuilder.h
@@ -33,6 +33,7 @@ class StringRef;
 class AAManager;
 class TargetMachine;
 class ModuleSummaryIndex;
+class MachineModuleInfo;
 template <typename T> class IntrusiveRefCntPtr;
 namespace vfs {
 class FileSystem;
@@ -106,6 +107,8 @@ class PassBuilder {
   PipelineTuningOptions PTO;
   std::optional<PGOOptions> PGOOpt;
   PassInstrumentationCallbacks *PIC;
+  std::unique_ptr<MachineModuleInfo> MMI;
+  MachineModuleInfo *MMIRefPtr; // As reference to MMI
 
 public:
   /// A struct to capture parsed pass pipeline names.
@@ -626,6 +629,16 @@ class PassBuilder {
   void invokePipelineEarlySimplificationEPCallbacks(ModulePassManager &MPM,
                                                     OptimizationLevel Level);
 
+  MachineModuleInfo &getMachineModuleInfo() { return *MMIRefPtr; }
+
+  std::unique_ptr<MachineModuleInfo> takeMachineModuleInfo() {
+    auto Ptr = std::move(MMI);
+    MMI = nullptr;
+    return Ptr;
+  }
+
+  bool hasMachineModuleInfoOwnership() const { return MMI != nullptr; }
+
   static bool checkParametrizedPassName(StringRef Name, StringRef PassName) {
     if (!Name.consume_front(PassName))
       return false;
diff --git a/llvm/include/llvm/Target/TargetMachine.h b/llvm/include/llvm/Target/TargetMachine.h
index ceb371bdc73480..f0f5cf3d443765 100644
--- a/llvm/include/llvm/Target/TargetMachine.h
+++ b/llvm/include/llvm/Target/TargetMachine.h
@@ -456,7 +456,7 @@ class LLVMTargetMachine : public TargetMachine {
   virtual Error buildCodeGenPipeline(ModulePassManager &, raw_pwrite_stream &,
                                      raw_pwrite_stream *, CodeGenFileType,
                                      const CGPassBuilderOption &,
-                                     PassInstrumentationCallbacks *) {
+                                     PassBuilder &) {
     return make_error<StringError>("buildCodeGenPipeline is not overridden",
                                    inconvertibleErrorCode());
   }
diff --git a/llvm/lib/CodeGen/MachinePassManager.cpp b/llvm/lib/CodeGen/MachinePassManager.cpp
index 2763193b2c306b..21449dd6d86f97 100644
--- a/llvm/lib/CodeGen/MachinePassManager.cpp
+++ b/llvm/lib/CodeGen/MachinePassManager.cpp
@@ -71,7 +71,7 @@ bool MachineFunctionAnalysisManagerModuleProxy::Result::invalidate(
 
 PreservedAnalyses
 ModuleToMachineFunctionPassAdaptor::run(Module &M, ModuleAnalysisManager &AM) {
-  auto &MMI = AM.getResult<MachineModuleAnalysis>(M).getMMI();
+  auto &MMI = getMachineModuleInfo();
   MachineFunctionAnalysisManager &MFAM =
       AM.getResult<MachineFunctionAnalysisManagerModuleProxy>(M).getManager();
   PassInstrumentation PI = AM.getResult<PassInstrumentationAnalysis>(M);
diff --git a/llvm/lib/Passes/PassBuilder.cpp b/llvm/lib/Passes/PassBuilder.cpp
index 57975e34d4265b..3b2666059627bf 100644
--- a/llvm/lib/Passes/PassBuilder.cpp
+++ b/llvm/lib/Passes/PassBuilder.cpp
@@ -92,6 +92,7 @@
 #include "llvm/CodeGen/JMCInstrumenter.h"
 #include "llvm/CodeGen/LowerEmuTLS.h"
 #include "llvm/CodeGen/MIRPrinter.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/MachinePassManager.h"
 #include "llvm/CodeGen/SafeStack.h"
 #include "llvm/CodeGen/SelectOptimize.h"
@@ -397,10 +398,13 @@ class RequireAllMachineFunctionPropertiesPass
 PassBuilder::PassBuilder(TargetMachine *TM, PipelineTuningOptions PTO,
                          std::optional<PGOOptions> PGOOpt,
                          PassInstrumentationCallbacks *PIC)
-    : TM(TM), PTO(PTO), PGOOpt(PGOOpt), PIC(PIC) {
+    : TM(TM), PTO(PTO), PGOOpt(PGOOpt), PIC(PIC), MMIRefPtr(nullptr) {
   bool ShouldPopulateClassToPassNames = PIC && shouldPopulateClassToPassNames();
-  if (TM)
+  if (TM) {
     TM->registerPassBuilderCallbacks(*this, ShouldPopulateClassToPassNames);
+    MMI.reset(new MachineModuleInfo(static_cast<LLVMTargetMachine *>(TM)));
+    MMIRefPtr = MMI.get();
+  }
   if (ShouldPopulateClassToPassNames) {
 #define MODULE_PASS(NAME, CREATE_PASS)                                         \
   PIC->addClassToPassName(decltype(CREATE_PASS)::name(), NAME);
@@ -1412,7 +1416,16 @@ Error PassBuilder::parseModulePass(ModulePassManager &MPM,
       MachineFunctionPassManager MFPM;
       if (auto Err = parseMachinePassPipeline(MFPM, InnerPipeline))
         return Err;
-      MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM)));
+
+      if (MMI) {
+        MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM),
+                                                             std::move(MMI)));
+        MMI = nullptr;
+      } else {
+        MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM),
+                                                             *MMIRefPtr));
+      }
+
       return Error::success();
     }
     if (auto Params = parseFunctionPipelineName(Name)) {
diff --git a/llvm/lib/Passes/PassRegistry.def b/llvm/lib/Passes/PassRegistry.def
index 41f16d0915bf23..a685893868fa4b 100644
--- a/llvm/lib/Passes/PassRegistry.def
+++ b/llvm/lib/Passes/PassRegistry.def
@@ -23,6 +23,7 @@ MODULE_ANALYSIS("collector-metadata", CollectorMetadataAnalysis())
 MODULE_ANALYSIS("inline-advisor", InlineAdvisorAnalysis())
 MODULE_ANALYSIS("ir-similarity", IRSimilarityAnalysis())
 MODULE_ANALYSIS("lcg", LazyCallGraphAnalysis())
+MODULE_ANALYSIS("machine-module", MachineModuleAnalysis(*MMIRefPtr))
 MODULE_ANALYSIS("module-summary", ModuleSummaryIndexAnalysis())
 MODULE_ANALYSIS("no-op-module", NoOpModuleAnalysis())
 MODULE_ANALYSIS("pass-instrumentation", PassInstrumentationAnalysis(PIC))
diff --git a/llvm/lib/Target/X86/X86CodeGenPassBuilder.cpp b/llvm/lib/Target/X86/X86CodeGenPassBuilder.cpp
index 453754381034e3..c1e37a3188a7b7 100644
--- a/llvm/lib/Target/X86/X86CodeGenPassBuilder.cpp
+++ b/llvm/lib/Target/X86/X86CodeGenPassBuilder.cpp
@@ -23,8 +23,8 @@ class X86CodeGenPassBuilder : public CodeGenPassBuilder<X86CodeGenPassBuilder> {
 public:
   explicit X86CodeGenPassBuilder(LLVMTargetMachine &TM,
                                  const CGPassBuilderOption &Opts,
-                                 PassInstrumentationCallbacks *PIC)
-      : CodeGenPassBuilder(TM, Opts, PIC) {}
+                                 PassBuilder &PB)
+      : CodeGenPassBuilder(TM, Opts, PB) {}
   void addPreISel(AddIRPass &addPass) const;
   void addAsmPrinter(AddMachinePass &, CreateMCStreamer) const;
   Error addInstSelector(AddMachinePass &) const;
@@ -48,8 +48,7 @@ Error X86CodeGenPassBuilder::addInstSelector(AddMachinePass &) const {
 
 Error X86TargetMachine::buildCodeGenPipeline(
     ModulePassManager &MPM, raw_pwrite_stream &Out, raw_pwrite_stream *DwoOut,
-    CodeGenFileType FileType, const CGPassBuilderOption &Opt,
-    PassInstrumentationCallbacks *PIC) {
-  auto CGPB = X86CodeGenPassBuilder(*this, Opt, PIC);
+    CodeGenFileType FileType, const CGPassBuilderOption &Opt, PassBuilder &PB) {
+  auto CGPB = X86CodeGenPassBuilder(*this, Opt, PB);
   return CGPB.buildPipeline(MPM, Out, DwoOut, FileType);
 }
diff --git a/llvm/lib/Target/X86/X86TargetMachine.h b/llvm/lib/Target/X86/X86TargetMachine.h
index 4e7ded16729d07..d47786623c3c69 100644
--- a/llvm/lib/Target/X86/X86TargetMachine.h
+++ b/llvm/lib/Target/X86/X86TargetMachine.h
@@ -61,7 +61,7 @@ class X86TargetMachine final : public LLVMTargetMachine {
   Error buildCodeGenPipeline(ModulePassManager &, raw_pwrite_stream &,
                              raw_pwrite_stream *, CodeGenFileType,
                              const CGPassBuilderOption &,
-                             PassInstrumentationCallbacks *) override;
+                             PassBuilder &) override;
 
   bool isJIT() const { return IsJIT; }
 
diff --git a/llvm/tools/llc/NewPMDriver.cpp b/llvm/tools/llc/NewPMDriver.cpp
index 6ae1b8db5e115c..1a1abc9d29c106 100644
--- a/llvm/tools/llc/NewPMDriver.cpp
+++ b/llvm/tools/llc/NewPMDriver.cpp
@@ -113,8 +113,6 @@ int llvm::compileModuleWithNewPM(
   Opt.DebugPM = DebugPM;
   Opt.RegAlloc = RegAlloc;
 
-  MachineModuleInfo MMI(&LLVMTM);
-
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI(Context, Opt.DebugPM);
   SI.registerCallbacks(PIC);
@@ -134,9 +132,9 @@ int llvm::compileModuleWithNewPM(
   PB.crossRegisterProxies(LAM, FAM, CGAM, MAM, &MFAM);
 
   FAM.registerPass([&] { return TargetLibraryAnalysis(TLII); });
-  MAM.registerPass([&] { return MachineModuleAnalysis(MMI); });
 
   ModulePassManager MPM;
+  auto &MMI = PB.getMachineModuleInfo();
 
   if (!PassPipeline.empty()) {
     // Construct a custom pass pipeline that starts after instruction
@@ -153,13 +151,13 @@ int llvm::compileModuleWithNewPM(
     MachineFunctionPassManager MFPM;
     MFPM.addPass(PrintMIRPass(*OS));
     MFPM.addPass(FreeMachineFunctionPass());
-    MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM)));
+    MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM), MMI));
 
     if (MIR->parseMachineFunctions(*M, MMI))
       return 1;
   } else {
     ExitOnErr(LLVMTM.buildCodeGenPipeline(
-        MPM, *OS, DwoOut ? &DwoOut->os() : nullptr, FileType, Opt, &PIC));
+        MPM, *OS, DwoOut ? &DwoOut->os() : nullptr, FileType, Opt, PB));
   }
 
   if (PrintPipelinePasses) {
diff --git a/llvm/unittests/CodeGen/PassManagerTest.cpp b/llvm/unittests/CodeGen/PassManagerTest.cpp
index 4283eb01a9c8f2..d63ec435600800 100644
--- a/llvm/unittests/CodeGen/PassManagerTest.cpp
+++ b/llvm/unittests/CodeGen/PassManagerTest.cpp
@@ -173,8 +173,6 @@ TEST_F(PassManagerTest, Basic) {
   LLVMTargetMachine *LLVMTM = static_cast<LLVMTargetMachine *>(TM.get());
   M->setDataLayout(TM->createDataLayout());
 
-  MachineModuleInfo MMI(LLVMTM);
-
   LoopAnalysisManager LAM;
   FunctionAnalysisManager FAM;
   CGSCCAnalysisManager CGAM;
@@ -189,7 +187,6 @@ TEST_F(PassManagerTest, Basic) {
   PB.crossRegisterProxies(LAM, FAM, CGAM, MAM, &MFAM);
 
   FAM.registerPass([&] { return TestFunctionAnalysis(); });
-  MAM.registerPass([&] { return MachineModuleAnalysis(MMI); });
   MFAM.registerPass([&] { return TestMachineFunctionAnalysis(); });
 
   int Count = 0;
@@ -197,12 +194,13 @@ TEST_F(PassManagerTest, Basic) {
 
   ModulePassManager MPM;
   MachineFunctionPassManager MFPM;
+  auto &MMI = PB.getMachineModuleInfo();
   MPM.addPass(TestMachineModulePass(Count, Counts));
   MPM.addPass(createModuleToMachineFunctionPassAdaptor(
-      TestMachineFunctionPass(Count, Counts)));
+      TestMachineFunctionPass(Count, Counts), MMI));
   MPM.addPass(TestMachineModulePass(Count, Counts));
   MFPM.addPass(TestMachineFunctionPass(Count, Counts));
-  MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM)));
+  MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM), MMI));
 
   MPM.run(*M, MAM);
 
diff --git a/llvm/unittests/MIR/PassBuilderCallbacksTest.cpp b/llvm/unittests/MIR/PassBuilderCallbacksTest.cpp
index 8e3738dc919209..be37aa7802c2c3 100644
--- a/llvm/unittests/MIR/PassBuilderCallbacksTest.cpp
+++ b/llvm/unittests/MIR/PassBuilderCallbacksTest.cpp
@@ -528,8 +528,8 @@ TEST_F(MachineFunctionCallbacksTest, InstrumentedFreeMFPass) {
               runAfterPass(HasNameRegex("FreeMachineFunctionPass"), _, _))
       .Times(0);
 
-  MPM.addPass(
-      createModuleToMachineFunctionPassAdaptor(FreeMachineFunctionPass()));
+  MPM.addPass(createModuleToMachineFunctionPassAdaptor(
+      FreeMachineFunctionPass(), *MMI));
   MPM.run(*M, MAM);
 }
 
@@ -568,7 +568,7 @@ TEST_F(MachineFunctionCallbacksTest, InstrumentedFreeMFPass2) {
 
   MachineFunctionPassManager MFPM;
   MFPM.addPass(FreeMachineFunctionPass());
-  MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM)));
+  MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM), *MMI));
   MPM.run(*M, MAM);
 }
 

@paperchalice paperchalice force-pushed the ownership branch 2 times, most recently from 6f09754 to ca4c589 Compare April 1, 2024 13:29
@@ -106,6 +107,8 @@ class PassBuilder {
PipelineTuningOptions PTO;
std::optional<PGOOptions> PGOOpt;
PassInstrumentationCallbacks *PIC;
std::unique_ptr<MachineModuleInfo> MMI;
MachineModuleInfo *MMIRefPtr; // As reference to MMI
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this design choice to use a unique_ptr and its reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR tries to let the first ModuleToMachineFunctionPassAdaptor in pipeline owns all machine functions, but before the first ModuleToMachineFunctionPassAdaptor, we need somewhere store the initial MachineModuleInfo.

if (!MFPM.isEmpty())
MPM.addPass(createModuleToMachineFunctionPassAdaptor(std::move(MFPM)));
if (!MFPM.isEmpty()) {
if (PB.PB.hasMachineModuleInfoOwnership()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why PB.PB here and a few more places below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have PassBuilder and CodeGenPassBuilder, here PassBuilder may transfer the ownership of MachineModuleInfo.

Thus `MachineFunction`s are no longer analysis results, but still
keep `MachineModuleAnalysis` so "machine module pass" can work.
@paperchalice
Copy link
Contributor Author

shared_ptr seems better here.

@arsenm
Copy link
Contributor

arsenm commented Apr 2, 2024

shared_ptr seems better here.

Why? I generally think shared_ptr is not the answer

@paperchalice
Copy link
Contributor Author

shared_ptr seems better here.

Why? I generally think shared_ptr is not the answer

There might be several <IR>ToMachineFunctionPassAdaptors in pipeline, there is no benefit in considering ownership among these adapters, just let them share the MachineModuleInfo.

@aeubanks
Copy link
Contributor

I meant in the discourse post that we should get rid of MachineModuleInfo, and have a separate Function analysis that returns a MachineFunction (maybe need some level of indirection like a std::unique_ptr). So basically the FunctionAnalysisManager's analysis cache would own the MachineFunction.

@paperchalice
Copy link
Contributor Author

Will try to revive a function analysis that owns machine function.

@paperchalice paperchalice deleted the ownership branch April 13, 2024 11:04
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

5 participants